Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces a comprehensive Software Catalog feature enabling users to browse, install, and manage software items (OS images and applications) across the platform. It includes new navigation routes, API integrations, multi-step installation/editing wizards, dynamic form handling, and type definitions generated from OpenAPI specifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CatalogPage as Catalog UI
participant API as Catalog API
participant FormWizard as Install Wizard
participant PatchAPI as Patch API
participant Device/Fleet
User->>CatalogPage: Browse catalog items
CatalogPage->>API: Fetch catalog items (filtered)
API-->>CatalogPage: Return CatalogItem list
CatalogPage->>User: Display items
User->>CatalogPage: Select item & click Install
CatalogPage->>FormWizard: Pass CatalogItem
User->>FormWizard: Specify version/channel (Specifications)
User->>FormWizard: Pick target device/fleet (SelectTarget)
FormWizard->>API: Fetch current OS/apps from target
API-->>FormWizard: Return current state
alt App Deployment
User->>FormWizard: Configure app (AppConfig)
FormWizard->>User: Render dynamic form from schema
end
User->>FormWizard: Review changes (Review)
FormWizard->>User: Show patches preview
User->>FormWizard: Submit
FormWizard->>PatchAPI: Apply patches (setImageRef, addApplication, setLabels)
PatchAPI->>Device/Fleet: Patch resource
Device/Fleet-->>PatchAPI: Updated resource
PatchAPI-->>FormWizard: Success
FormWizard->>User: Show success page
sequenceDiagram
participant User
participant EditWizard as Edit Wizard
participant CatalogAPI as Catalog API
participant VersionGraph as Update Graph
participant PatchAPI as Patch API
participant Device/Fleet
User->>EditWizard: Open deployed app/OS edit
EditWizard->>CatalogAPI: Fetch CatalogItem + versions
CatalogAPI-->>EditWizard: Return item with versions
EditWizard->>EditWizard: Compute update candidates
User->>VersionGraph: View version upgrade graph
VersionGraph->>VersionGraph: Render nodes (current, available, deprecated)
VersionGraph->>VersionGraph: Draw edges (replaces, skips, skipRange)
VersionGraph->>User: Display interactive graph
User->>VersionGraph: Select target version
VersionGraph->>EditWizard: onSelectionChange(version)
EditWizard->>User: Update form with selected version
alt OS Update
User->>EditWizard: Review OS replacement specs
else App Update
User->>EditWizard: Reconfigure app (AppConfig)
EditWizard->>EditWizard: Validate schema
end
User->>EditWizard: Submit update
EditWizard->>EditWizard: Generate patches (new image/app spec + labels)
EditWizard->>PatchAPI: Send patches
PatchAPI->>Device/Fleet: Apply patches
Device/Fleet-->>PatchAPI: Updated
PatchAPI-->>EditWizard: Success
EditWizard->>User: Navigate to resource
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
rawagner
force-pushed
the
catalog
branch
10 times, most recently
from
a9fdb26 to
0632f21
Compare
libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx
Outdated
Show resolved
Hide resolved
| ); | ||
| } | ||
| } else if (values.target === 'device') { | ||
| if (!values.device?.spec?.os?.image) { |
There was a problem hiding this comment.
When choosing to deploy an application on a fleetless device without an OS in its spec, the banner is displayed and this is not correct.
I suspect the same would happen if I try to deploy an app to a fleet that does not specify the OS yet.
There was a problem hiding this comment.
these were mostly placeholders..the alerts are now updated and should be correct
There was a problem hiding this comment.
The issue that we're deploying an application to a device that didn't specify the OS is still present.
Same as before, I think we need to check that the catalogItem.type is for an OS.
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/Device/DevicesPage/useDevices.ts
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| if (values.target === 'fleet') { | ||
| const numOfDevices = `${values.fleet?.status?.devicesSummary?.total || 0}`; | ||
| if (!values.fleet?.spec.template.spec.os?.image) { |
There was a problem hiding this comment.
I think we should check the catalogItem.type and only show this warning when it's an OS that's being added.
There was a problem hiding this comment.
we have the isOs prop and only then render the alerts..but you are right that we can do that based on the catalogItem prop.
Fixed
| ); | ||
| } | ||
| } else if (values.target === 'device') { | ||
| if (!values.device?.spec?.os?.image) { |
There was a problem hiding this comment.
The issue that we're deploying an application to a device that didn't specify the OS is still present.
Same as before, I think we need to check that the catalogItem.type is for an OS.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
libs/ui-components/src/components/common/FlightCtlWizardFooter.tsx (1)
57-63:⚠️ Potential issue | 🟡 Minor
Closebutton on the read-only review step ignoresonCancel.The Cancel button (line 88) correctly uses
onCancel ?? (() => navigate(-1)), but theClosebutton rendered on the read-only submit step (line 60) still unconditionally callsnavigate(-1). If a modal-hosted wizard passesonCancelto close the modal, clickingCloseon that step will attempt to navigate instead.🐛 Proposed fix
- <Button variant="primary" onClick={() => navigate(-1)}> + <Button variant="primary" onClick={onCancel ?? (() => navigate(-1))}> {t('Close')} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/FlightCtlWizardFooter.tsx` around lines 57 - 63, The Close button rendered in the isSubmitStep branch currently calls navigate(-1) unconditionally; update that branch so the Button's onClick uses the same cancellation handler as the Cancel button (i.e., onCancel ?? (() => navigate(-1))). Locate the isSubmitStep conditional and the primaryBtn assignment inside FlightCtlWizardFooter (where primaryBtn and Button are set) and replace the hardcoded navigate(-1) with the nullable onCancel fallback to ensure modal-hosted wizards can close via the provided onCancel.libs/types/scripts/openapi-typescript.js (2)
51-53:⚠️ Potential issue | 🟡 MinorError message doesn't mention the new
alphacoremode.The error at Line 52 still reads
"Use 'core' or 'imagebuilder'", butalphacoreis now a valid mode.✏️ Proposed fix
- throw new Error(`Unknown mode: ${mode}. Use 'core' or 'imagebuilder'`); + throw new Error(`Unknown mode: ${mode}. Use 'core', 'alphacore', or 'imagebuilder'`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 51 - 53, The thrown Error when checking if (!config[mode]) still instructs "Use 'core' or 'imagebuilder'" even though alphacore is supported; update the Error message in that validation block to include 'alphacore' (or better, derive the allowed modes from Object.keys(config) to keep it accurate) so the message reflects the actual valid modes for the variables config and mode.
126-131:⚠️ Potential issue | 🟡 MinorInitial cleanup doesn't remove
alphaortmp-alpha-typesdirectories.The
Promise.allblock cleans upmodels,imagebuilder,tmp-types, andtmp-imagebuilder-types, but omits the alpha equivalents. For a clean generation run,alphaandtmp-alpha-typesshould be included.✏️ Proposed fix
await Promise.all([ rimraf(path.join(rootDir, 'models')), rimraf(path.join(rootDir, 'imagebuilder')), + rimraf(path.join(rootDir, 'alpha')), rimraf(path.join(rootDir, 'tmp-types')), rimraf(path.join(rootDir, 'tmp-imagebuilder-types')), + rimraf(path.join(rootDir, 'tmp-alpha-types')), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 126 - 131, The Promise.all cleanup currently calls rimraf for 'models', 'imagebuilder', 'tmp-types', and 'tmp-imagebuilder-types' but misses the alpha equivalents; update the Promise.all invocation to also call rimraf(path.join(rootDir, 'alpha')) and rimraf(path.join(rootDir, 'tmp-alpha-types')) so both the alpha and temporary alpha directories are removed before generation—locate the Promise.all block and add those two rimraf calls alongside the existing ones (using the same rimraf and rootDir variables).libs/ui-components/src/components/Table/Table.tsx (1)
80-91:⚠️ Potential issue | 🟡 Minor
singleSelectandonSelectAllare not mutually exclusive — both header cells could render simultaneously.If both
singleSelectandonSelectAllare truthy, both a select-all checkbox<Th>and a single-select<Th>will render, producing two selection columns. These modes are semantically contradictory. Consider guarding or documenting that they should not be combined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Table/Table.tsx` around lines 80 - 91, The header currently renders both selection cells when singleSelect and onSelectAll are truthy; update the rendering logic in Table.tsx (around the Tr that renders Th) to make them mutually exclusive — e.g., replace the two independent conditions with a single conditional that prefers onSelectAll (render the select-all Th when onSelectAll is provided) else renders the singleSelect Th; also add a console.warn or prop validation inside the same component to surface misuse when both props are passed.
♻️ Duplicate comments (2)
libs/types/scripts/openapi-typescript.js (1)
77-96:⚠️ Potential issue | 🟡 MinorStale comments and variable names referencing "imagebuilder" instead of "alpha".
Lines 78, 87, and 90 still reference "Image builder" and use
imagebuilderDiras the variable name, but this block handles the alpha core API types targeting the../alphadirectory. This was copy-pasted from the imagebuilder block without updating the comments/names.✏️ Proposed fix
} else if (mode === ALPHA_CORE_API) { - // Image builder types need to be fixed before they can be moved to their final location + // Alpha types need to be fixed before they can be moved to their final location await rimraf(finalDir); const modelsDir = path.join(output, 'models'); if (fs.existsSync(modelsDir)) { await copyDir(modelsDir, finalDir); } console.log(`Fixing references to core API types...`); await fixCoreReferences(finalDir); - // Copy the generated index.ts to imagebuilder/index.ts + // Copy the generated index.ts to alpha/index.ts const indexPath = path.join(output, 'index.ts'); if (fs.existsSync(indexPath)) { - const imagebuilderDir = path.resolve(__dirname, '../alpha'); - if (!fs.existsSync(imagebuilderDir)) { - fs.mkdirSync(imagebuilderDir, { recursive: true }); + const alphaDir = path.resolve(__dirname, '../alpha'); + if (!fs.existsSync(alphaDir)) { + fs.mkdirSync(alphaDir, { recursive: true }); } - await fsPromises.copyFile(indexPath, path.join(imagebuilderDir, 'index.ts')); + await fsPromises.copyFile(indexPath, path.join(alphaDir, 'index.ts')); } await rimraf(output);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 77 - 96, The comments and variable names in the ALPHA_CORE_API branch are stale references to "imagebuilder" — update the inline comment and the variable imagebuilderDir to reflect "alpha" (e.g., alphaDir) and adjust the comment text near the block handling mode === ALPHA_CORE_API and the copy operation that uses indexPath/output so they clearly state they target the ../alpha directory; ensure any mkdirSync/fsPromises.copyFile calls remain unchanged but use the renamed symbol (alphaDir) and update the log/comment that currently says "Copy the generated index.ts to imagebuilder/index.ts" to "Copy the generated index.ts to alpha/index.ts".libs/ui-components/src/components/Catalog/CatalogPage.tsx (1)
208-208:⚠️ Potential issue | 🟠 Major
filterIsEmptyis alwaysfalse— empty array is truthy.
catalogFilter.itemTypeis initialized as[]byuseCatalogFilter, so!catalogFilter.itemTypeis alwaysfalse. The empty state will never distinguish between "no filters active" and "filters active but no results."🐛 Proposed fix
- const filterIsEmpty = !catalogFilter.itemType; + const filterIsEmpty = catalogFilter.itemType.length === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx` at line 208, filterIsEmpty incorrectly uses !catalogFilter.itemType which is always false because catalogFilter.itemType is initialized to an array; update the check to detect an empty array (e.g., Array.isArray(catalogFilter.itemType) ? catalogFilter.itemType.length === 0 : !catalogFilter.itemType) so filterIsEmpty reflects "no filters active"; change the variable computation in CatalogPage (filterIsEmpty) and keep the rest of the logic that relies on it unchanged.
🟠 Major comments (16)
libs/ui-components/src/components/Fleet/useFleets.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟠 Major
limitis declared but never forwarded — endpoint always uses hardcodedPAGE_SIZE.
getFleetsEndpoint(lines 53–60) does not accept or uselimit; it unconditionally setslimit: \${PAGE_SIZE}`. Any caller that passeslimitviauseFleets/useFleetsEndpoint` will have it silently discarded, making the public API surface misleading.Wire the field through or remove it until the implementation is ready.
🔧 Proposed fix — thread `limit` through `getFleetsEndpoint`
const getFleetsEndpoint = ({ name, addDevicesSummary, nextContinue, + limit, }: { name?: string; addDevicesSummary?: boolean; nextContinue?: string; + limit?: number; }) => { const params = new URLSearchParams({ - limit: `${PAGE_SIZE}`, + limit: `${limit ?? PAGE_SIZE}`, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Fleet/useFleets.ts` at line 18, The `limit` parameter is declared but never forwarded into the request - update getFleetsEndpoint to accept and use the incoming `limit` (instead of unconditionally using `PAGE_SIZE`), e.g. read `limit` from the function args and set the request `limit` to that value or fall back to `PAGE_SIZE` when `limit` is undefined; also propagate the `limit` parameter from `useFleets` / `useFleetsEndpoint` into the call to `getFleetsEndpoint` and adjust any type signatures so `limit?: number` is honored across these functions (keep `PAGE_SIZE` as the default fallback).libs/ui-components/src/components/Fleet/FleetRow.tsx-19-23 (1)
19-23:⚠️ Potential issue | 🟠 MajorRequire a delete handler when delete is enabled.
Line 19 makes
onDeleteClickoptional, but Lines 62-65 still add a clickable delete action whencanDeleteis true. This can render a non-functional delete action if a caller forgets the handler.Suggested fix
- if (canDelete) { + if (canDelete && onDeleteClick) { actions.push({ title: t('Delete fleet'), onClick: onDeleteClick, tooltipProps: isManaged ? { content: t( "This fleet is managed by a resource sync and cannot be directly deleted. Either remove this fleet's definition from the resource sync configuration, or delete the resource sync first.", ), } : undefined, isAriaDisabled: isManaged, }); }Also applies to: 50-51, 62-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Fleet/FleetRow.tsx` around lines 19 - 23, The component currently allows canDelete without a handler which can render a dead delete action; update FleetRow so that delete rendering and behavior require a handler by either making onDeleteClick non-optional in the props when canDelete is used or, at minimum, guard the JSX and only render the delete action when both canDelete and onDeleteClick are truthy (e.g., change the conditional that renders the delete button to use canDelete && onDeleteClick), and optionally add a runtime invariant (throw or console.warn) inside the FleetRow function if canDelete is true but onDeleteClick is missing to fail-fast; reference the onDeleteClick and canDelete props and the delete action render logic in FleetRow.apps/ocp-plugin/webpack.config.ts-53-59 (1)
53-59:⚠️ Potential issue | 🟠 MajorFix double-dot in image filenames.
Webpack 5's built-in
asset/resourcetype includes the leading dot in the[ext]template substitution (e.g.,.svg). Withfilename: 'assets/[name].[ext]', images will be output asassets/logo..svginstead ofassets/logo.svg. The fonts rule correctly usesfile-loader(where[ext]excludes the dot), but the images rule needs adjustment.Fix
generator: { - filename: 'assets/[name].[ext]', + filename: 'assets/[name][ext]', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ocp-plugin/webpack.config.ts` around lines 53 - 59, The image asset rule's generator.filename uses 'assets/[name].[ext]' which produces a double-dot because Webpack's [ext] includes the leading dot; update the generator.filename in the image rule (the object with test: /\.(jpg|jpeg|png|gif|svg)$/i and generator) to use 'assets/[name][ext]' (remove the extra dot) so outputs become assets/logo.svg instead of assets/logo..svg.libs/ui-components/src/components/Catalog/InstallWizard/utils.ts-50-51 (1)
50-51:⚠️ Potential issue | 🟠 Major
lodash/mergemutatesdefaultConfigin-place, corrupting the sourceCatalogItemdata.On Line 51, when
defaultConfigis truthy it is a direct reference fromcatalogItem.spec.versions[].configorcatalogItem.spec.defaults.config.merge(defaultConfig || {}, appConfig)mutates that object. Subsequent calls or re-renders will see the corrupted data.Always merge into a fresh object:
🐛 Proposed fix
- defaultConfig = merge(defaultConfig || {}, appConfig); + defaultConfig = merge({}, defaultConfig || {}, appConfig);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/utils.ts` around lines 50 - 51, The current use of lodash/merge mutates the source object (defaultConfig), corrupting the original CatalogItem data; update the merges to use a fresh target object so we don't mutate inputs: for the defaultConfig merge (the line with merge(defaultConfig || {}, appConfig)) change it to merge into a new object (e.g., pass an empty object as the first arg) so you call merge with a fresh target and the sources defaultConfig and appConfig; likewise ensure the formValues merge (merge(formValues, appConfig)) does not unintentionally mutate external state by merging into a new object when needed.libs/ui-components/src/components/Catalog/const.ts-13-15 (1)
13-15:⚠️ Potential issue | 🟠 Major
getAppVolumeNameproduces a leading dot whenappNameis undefined or empty.When
appNameisundefinedor'', the template literal yields".volumeName.label"— a leading dot that results in malformed label keys like.myVolume.volume.catalog.flightctl.io/item. The function either needs to requireappName: string(non-optional) or conditionally omit the prefix.🐛 Proposed fix
export const getAppVolumeName = (appName: string | undefined, volumeName: string, label: string) => { - return `${appName || ''}.${volumeName}.${label}`; + const prefix = appName ? `${appName}.` : ''; + return `${prefix}${volumeName}.${label}`; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/const.ts` around lines 13 - 15, getAppVolumeName currently produces a leading dot when appName is undefined/empty; update the function (getAppVolumeName) to either make appName required or, better, conditionally include the appName prefix: if appName is truthy use `${appName}.${volumeName}.${label}`, otherwise return `${volumeName}.${label}` (or similarly trim empty/whitespace appName before deciding) so no leading dot appears and label keys remain valid.libs/ui-components/src/components/DynamicForm/FormWidget.tsx-91-94 (1)
91-94:⚠️ Potential issue | 🟠 MajorSelect widget coerces enum values to strings and breaks typed schemas.
Casting values to
stringchanges numeric/boolean enum values in form state, which can invalidate payload types.🔧 Proposed fix (preserve original enum value types)
- selected={value as string} + selected={value as string | number | undefined} onSelect={(_event, val) => { - onChange(val); + onChange(val as string | number); setIsOpen(false); }} @@ - {enumOptions.map((option) => ( - <SelectOption key={String(option.value as string)} value={option.value as string}> + {enumOptions.map((option) => ( + <SelectOption key={String(option.value)} value={option.value as string | number}> {option.label} </SelectOption> ))}Also applies to: 112-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/FormWidget.tsx` around lines 91 - 94, In FormWidget.tsx, stop coercing enum values to strings: remove the cast on selected and pass the original typed value through onSelect/onChange so numeric/boolean enum values remain their original types; update the onSelect handler (where it calls onChange(val) and setIsOpen(false)) to accept and forward the exact value type from the option (not a string) and ensure the selected prop uses the original value (not value as string); apply the same fix to the other occurrence around the onSelect/onChange block near lines 112-114 so form state preserves enum numeric/boolean types.libs/ui-components/src/components/DynamicForm/FormWidget.tsx-128-133 (1)
128-133:⚠️ Potential issue | 🟠 MajorNumber widget currently forces
0/NaNinto optional fields.Empty values are coerced to
0, and invalid input can emitNaN. Both can create unintended state and payload values.🔧 Proposed fix (preserve empty state, guard invalid numbers)
- const numValue = typeof value === 'number' ? value : value ? Number(value) : 0; + const parsed = value === '' || value === undefined || value === null ? undefined : Number(value); + const numValue = typeof value === 'number' ? value : Number.isFinite(parsed) ? parsed : undefined; const handleChange = (event: React.FormEvent<HTMLInputElement>) => { const val = (event.target as HTMLInputElement).value; - onChange(val === '' ? undefined : Number(val)); + if (val === '') { + onChange(undefined); + return; + } + const next = Number(val); + onChange(Number.isFinite(next) ? next : undefined); }; return ( <NumberInput id={id} - value={numValue} + value={numValue ?? ''} min={min} max={max}Also applies to: 136-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/FormWidget.tsx` around lines 128 - 133, The number widget is coercing empty/invalid input to 0/NaN; update the logic in the numValue calculation and handleChange so empty string or null/undefined preserves an undefined value (not 0) and invalid parses do not emit NaN — instead call onChange(undefined) for empty or non-numeric input and only call onChange(Number(parsed)) when Number(parsed) is a finite number; adjust the numValue derivation (currently using value, numValue) to display ''/undefined for empty state and apply the same guard in the other similar block around lines 136-143 to avoid propagating 0/NaN.libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx-122-122 (1)
122-122:⚠️ Potential issue | 🟠 MajorFix the Biome
useIterableCallbackReturnerror in the dedupe loop.The expression-bodied
forEachcallback returnsentriesMap.set(...), which triggers the lint error.🧹 Proposed fix
- directUpgradeEntries.forEach((entry) => entriesMap.set(entry.version, entry)); + directUpgradeEntries.forEach((entry) => { + entriesMap.set(entry.version, entry); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx` at line 122, The lint error is caused by the expression-bodied forEach callback returning the value from entriesMap.set(...); change the callback to a block-bodied function that returns void (or use an explicit for..of loop) so no value is returned. Replace directUpgradeEntries.forEach((entry) => entriesMap.set(entry.version, entry)); with either directUpgradeEntries.forEach((entry) => { entriesMap.set(entry.version, entry); }); or a for (const entry of directUpgradeEntries) { entriesMap.set(entry.version, entry); } in UpdateGraph.tsx (referencing directUpgradeEntries and entriesMap).libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx-227-252 (1)
227-252:⚠️ Potential issue | 🟠 MajorController/model are frozen to initial props due to empty
useMemodeps.The graph model and selection handler capture initial values only. If
updates,currentVersion,currentChannel, oronSelectionChangechange, the visualization becomes stale.♻️ Proposed fix (separate controller creation from model updates)
- const controller = React.useMemo(() => { + const controller = React.useMemo(() => { const newController = new Visualization(); newController.registerComponentFactory(customComponentFactory); newController.registerLayoutFactory(customLayoutFactory); newController.addEventListener(GRAPH_LAYOUT_END_EVENT, () => { newController.getGraph().fit(80); @@ newController.addEventListener(SELECTION_EVENT, (ids: string[]) => { @@ }); - - const model = buildTopologyModel(currentVersion, updates, currentChannel); - newController.fromModel(model, false); - return newController; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [onSelectionChange]); + + React.useEffect(() => { + const model = buildTopologyModel(currentVersion, updates, currentChannel); + controller.fromModel(model, false); + }, [controller, currentVersion, updates, currentChannel]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx` around lines 227 - 252, The controller and its model are frozen because useMemo has an empty dep array; keep creating the Visualization instance and registering static factories once (Visualization, customComponentFactory, customLayoutFactory) but move dynamic work into effects: create the controller with React.useMemo(() => new Visualization(), []), register static listeners once, then add a React.useEffect that rebuilds the model (call buildTopologyModel(currentVersion, updates, currentChannel)) and calls controller.fromModel(model, false) whenever updates/currentVersion/currentChannel change; also ensure the selection callback uses the latest onSelectionChange by either storing onSelectionChange in a ref and reading ref.current inside the listener or by re-registering the SELECTION_EVENT handler in an effect that depends on onSelectionChange so the handler always calls the current onSelectionChange; reference symbols: controller/Visualization, buildTopologyModel, controller.fromModel, addEventListener(SELECTION_EVENT), and onSelectionChange.libs/ui-components/src/components/Catalog/InstallWizard/types.ts-10-13 (1)
10-13:⚠️ Potential issue | 🟠 MajorConsolidate
InstallSpecFormikto the canonical location intypes.ts.The type is currently defined in both
libs/ui-components/src/components/Catalog/InstallWizard/types.ts(lines 10–13) andlibs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx(lines 42–44). Remove the duplicate fromSpecificationsStep.tsx, export it fromtypes.ts, and update imports inVolumeImageField.tsxandCatalogItemDetails.tsxto import fromtypes.tsinstead ofSpecificationsStep.tsx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/types.ts` around lines 10 - 13, Remove the duplicate InstallSpecFormik type from SpecificationsStep.tsx and ensure the canonical definition in types.ts (InstallSpecFormik) is exported; then update all consumers (imports in VolumeImageField.tsx and CatalogItemDetails.tsx and any other files still importing from SpecificationsStep.tsx) to import InstallSpecFormik from the centralized types.ts instead of SpecificationsStep.tsx, and run a quick TypeScript build to confirm no remaining references to the duplicate remain.libs/types/package.json-20-24 (1)
20-24:⚠️ Potential issue | 🟠 Major
./alphaexport is added, butalphadirectory is not whitelisted in package files.The
exports["./alpha"].sourcefield points to./alpha/index.ts(which exists), butalphais missing from thefileswhitelist (lines 26–30). This breaks the source condition for consumers and tools that expect to resolve the source file from the published package.Add
"alpha"to the files array:Proposed fix
"files": [ "dist", "index.ts", - "imagebuilder" + "imagebuilder", + "alpha" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/package.json` around lines 20 - 24, The package.json adds an exports entry "./alpha" with source "./alpha/index.ts" but the "files" array lacks the "alpha" directory, so the source won't be published; update the package.json "files" array to include "alpha" (alongside any existing entries) so that the exports["./alpha"].source path is included in the published package and consumers/tools can resolve ./alpha/index.ts.libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx-204-232 (1)
204-232:⚠️ Potential issue | 🟠 Major
artifactTypeis typed incorrectly for translated labels.
artifactTypeis declared asCatalogItemArtifactType(an enum), but each branch assigns the result oft()which returns a translated string. This should be typed asstring.Proposed fix
- let artifactType: CatalogItemArtifactType; + let artifactType: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx` around lines 204 - 232, The variable artifactType is incorrectly typed as CatalogItemArtifactType while it's assigned translated strings from t(); change its type to string by updating the declaration for artifactType (the let artifactType declaration used in the switch on artifact.type) so all branches that call t(...) assign a string type; ensure any usages of artifactType (after the switch) expect a string rather than the enum.libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx-111-113 (1)
111-113:⚠️ Potential issue | 🟠 MajorAvoid casting a potentially missing version to
CatalogItemVersion.
find(...)can returnundefined; casting bypasses the check andEditAppModallater readscurrentVersion.version. Guard it before rendering.💡 Proposed fix
+ const selectedInstallVersion = installItem?.item.spec.versions.find( + (v) => v.version === installItem.version, + ); - {installItem?.item.spec.category === CatalogItemCategory.CatalogItemCategoryApplication && ( + {installItem?.item.spec.category === CatalogItemCategory.CatalogItemCategoryApplication && + selectedInstallVersion && ( <EditAppModal currentApps={spec?.applications} catalogItem={installItem.item} currentChannel={installItem.channel} exisingLabels={currentLabels} onClose={() => setInstallItem(undefined)} onSubmit={async (catalogItem, catalogItemVersion, channel, values) => { await onUpdateApp(catalogItem, catalogItemVersion, channel, values); setSelectedItem(undefined); }} - currentVersion={ - installItem.item.spec.versions.find((v) => v.version === installItem.version) as CatalogItemVersion - } + currentVersion={selectedInstallVersion} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx` around lines 111 - 113, The code unsafely casts the result of installItem.item.spec.versions.find(...) to CatalogItemVersion even though find can return undefined; update ResourceCatalogPage so currentVersion is validated before passing to EditAppModal (e.g., compute const currentVersion = installItem.item.spec.versions.find(v => v.version === installItem.version); if (!currentVersion) return or render a fallback/error; otherwise pass currentVersion as CatalogItemVersion), or change the prop to accept undefined and guard uses inside EditAppModal (ensure any access like currentVersion.version is only after the undefined check).libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx-252-262 (1)
252-262:⚠️ Potential issue | 🟠 Major
deploymentTargettype should bestring | undefined, notCatalogItemArtifactType | undefined.The field is assigned from
artifacts[0].uri(a string) and passed togetFullReferenceURIwhich expects its first parameter to be astring. Update the type declaration inlibs/ui-components/src/components/Catalog/InstallWizard/types.tsline 23.Proposed fix
--- a/libs/ui-components/src/components/Catalog/InstallWizard/types.ts +++ b/libs/ui-components/src/components/Catalog/InstallWizard/types.ts @@ -20,7 +20,7 @@ export type TargetPickerFormik = { export type InstallOsFormik = InstallSpecFormik & TargetPickerFormik & { - deploymentTarget: CatalogItemArtifactType | undefined; + deploymentTarget: string | undefined; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx` around lines 252 - 262, Change the deploymentTarget type from CatalogItemArtifactType | undefined to string | undefined in the InstallWizard types so it matches the actual assigned value (artifacts[0].uri) and the parameter expected by getFullReferenceURI; update the type alias/field in libs/ui-components/src/components/Catalog/InstallWizard/types.ts (the declaration for deploymentTarget used by SelectTargetStep.tsx and getFullReferenceURI) to string | undefined and ensure any places constructing or reading values.deploymentTarget use string semantics.libs/ui-components/src/components/DynamicForm/DynamicForm.tsx-6-7 (1)
6-7:⚠️ Potential issue | 🟠 MajorRemove import from internal
@rjsf/validator-ajv8/lib/validatorpath and useValidatorTypefrom public API.The
/lib/validatorpath is not part of the public API and can break on package updates. ImportValidatorTypefrom@rjsf/utilsinstead and use it to type the validator prop.Proposed fix
-import { RJSFSchema, RegistryWidgetsType, TemplatesType } from '@rjsf/utils'; +import { RJSFSchema, RegistryWidgetsType, TemplatesType, ValidatorType } from '@rjsf/utils'; import validator from '@rjsf/validator-ajv8'; import { CatalogItem } from '@flightctl/types/alpha'; -import AJV8Validator from '@rjsf/validator-ajv8/lib/validator'; ... - validator={validator as AJV8Validator<Record<string, unknown>, RJSFSchema, DynamicFormContext>} + validator={validator as ValidatorType<Record<string, unknown>, RJSFSchema, DynamicFormContext>}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/DynamicForm.tsx` around lines 6 - 7, Replace the internal import of AJV8Validator from '@rjsf/validator-ajv8/lib/validator' with the public ValidatorType from '@rjsf/utils' and update the DynamicForm component's validator prop/type accordingly: remove the AJV8Validator import, add an import for ValidatorType from '@rjsf/utils', and type the validator prop (or any related variables/functions) using ValidatorType so the component relies on the public API instead of the internal /lib path.libs/ui-components/src/components/Catalog/useCatalogs.ts-45-54 (1)
45-54:⚠️ Potential issue | 🟠 MajorOperator-precedence bug in multi-value field selector conditions.
On Lines 47 and 52,
>binds tighter than||, socategories?.length || 0 > 1is parsed ascategories?.length || (0 > 1)→categories?.length || false. The condition fires for any non-zero length instead of only length > 1. It happens to work today because the=== 1branch above consumes the single-element case, but it will break silently if the logic around it changes.The same applies to Line 52:
types.length || 0 > 1.🐛 Proposed fix
- if (categories.length === 1) { - parts.push(`spec.category==${categories[0]}`); - } else if (categories?.length || 0 > 1) { - parts.push(`spec.category in (${categories.join(',')})`); - } - if (types.length === 1) { - parts.push(`spec.type==${types[0]}`); - } else if (types.length || 0 > 1) { - parts.push(`spec.type in (${types?.join(',')})`); - } + if (categories.length === 1) { + parts.push(`spec.category==${categories[0]}`); + } else if (categories.length > 1) { + parts.push(`spec.category in (${categories.join(',')})`); + } + if (types.length === 1) { + parts.push(`spec.type==${types[0]}`); + } else if (types.length > 1) { + parts.push(`spec.type in (${types.join(',')})`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/useCatalogs.ts` around lines 45 - 54, The multi-value branch conditions in useCatalogs.ts are using incorrect operator precedence (e.g., categories?.length || 0 > 1 and types.length || 0 > 1) so they evaluate wrongly; update those checks to group the length fallback before the comparison — e.g., use (categories?.length || 0) > 1 and (types?.length || 0) > 1 — so the branches that push `spec.category in (...)` and `spec.type in (...)` only run when length is greater than one; adjust both the categories and types conditional expressions in the function that builds the parts array.
🟡 Minor comments (13)
libs/ui-components/src/components/form/FormSelect.tsx-30-31 (1)
30-31:⚠️ Potential issue | 🟡 MinorUse
??instead of||to guard against falsy-but-validReactNodevalues.
selectedLabelis typed asReact.ReactNode, which admits0,false, and''as legitimate render values. Using||silently drops those and falls back toitem.label.??(nullish coalescing) is correct here — it only falls back whenselectedLabelisnullorundefined.🛠️ Proposed fix
const getItemSelectLabel = (item: string | SelectItem) => - isItemObject(item) ? item.selectedLabel || item.label : item; + isItemObject(item) ? item.selectedLabel ?? item.label : item;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/form/FormSelect.tsx` around lines 30 - 31, The getItemSelectLabel helper drops valid falsy React nodes because it uses ||; update the fallback to use nullish coalescing so selectedLabel only falls back when null or undefined. In the getItemSelectLabel function (and any similar logic handling SelectItem.selectedLabel typed as React.ReactNode), replace the use of || with ?? so values like 0, false, and '' are preserved and only null/undefined trigger fallback to item.label.libs/ui-components/src/components/common/LabelsView.tsx-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUse
startsWithinstead ofincludesfor prefix matching
CATALOG_LABEL = 'catalog.flightctl.io/'is a key prefix, not an arbitrary substring. Using.includes()would incorrectly filter out keys wherecatalog.flightctl.io/appears mid-string (e.g. a hypotheticalmy-ns/catalog.flightctl.io/foo)..startsWith()expresses the intent precisely.🐛 Proposed fix
- const labelItems = Object.entries(labels || {}).filter(([key]) => !key.includes(CATALOG_LABEL)); + const labelItems = Object.entries(labels || {}).filter(([key]) => !key.startsWith(CATALOG_LABEL));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/LabelsView.tsx` at line 14, The filter in LabelsView.tsx uses includes on the label key which can match mid-string; update the labelItems creation (the const labelItems = Object.entries(...) line) to use key.startsWith(CATALOG_LABEL) and negate that (i.e., filter(([key]) => !key.startsWith(CATALOG_LABEL))) so it only excludes keys with the CATALOG_LABEL prefix; keep using the existing CATALOG_LABEL symbol for the prefix check.libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx-10-33 (1)
10-33:⚠️ Potential issue | 🟡 Minor
itemType/setItemTypeaccepted viaCatalogFilterspread but never consumed.By spreading the entire
CatalogFiltertype intoCatalogPageToolbarProps, the component requires callers to provideitemTypeandsetItemTypeeven though the component never renders or uses them. If a type-filter chip group is planned, consider adding a// TODOcomment; otherwise, narrow the props type to only what the component actually needs.♻️ Proposed fix (narrow the props if the filter UI isn't planned for this component)
-type CatalogPageToolbarProps = CatalogFilter & { +type CatalogPageToolbarProps = Pick<CatalogFilter, 'nameFilter' | 'setNameFilter'> & { pagination: PaginationDetails<CatalogItemList>; isUpdating: boolean; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPageToolbar.tsx` around lines 10 - 33, The CatalogPageToolbarProps currently spreads CatalogFilter which forces callers to pass itemType and setItemType even though CatalogPageToolbar never uses them; update the props by either (A) replacing "type CatalogPageToolbarProps = CatalogFilter & { ... }" with an explicit props type that only includes nameFilter, setNameFilter, pagination, and isUpdating, or (B) if you intend to add a type-filter UI later, keep the spread but add a clear TODO comment above CatalogPageToolbarProps referencing itemType and setItemType and mark them optional so callers aren’t forced to provide them; modify the type declaration and any affected usages accordingly (look for CatalogPageToolbarProps, CatalogFilter, itemType, setItemType, and CatalogPageToolbar).libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx-153-162 (1)
153-162:⚠️ Potential issue | 🟡 MinorDownload button has
type="submit"— likely should betype="button".The Download button at Line 155 uses
type="submit"inside a<FlightCtlForm>. This could trigger unintended form submission behavior (e.g., page reload or form handler invocation) in addition to theonClickhandler. Since this button only downloads the YAML content, it should betype="button".🐛 Proposed fix
<Button icon={<DownloadIcon />} - type="submit" + type="button" variant="secondary" className="pf-v6-u-ml-auto" aria-label={t('Download')} onClick={downloadYaml}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx` around lines 153 - 162, The Download Button in YamlEditorBase.tsx (the <Button> rendering Download with aria-label and onClick={downloadYaml}) is set to type="submit" and can unintentionally submit the surrounding FlightCtlForm; change the button's type to "button" so clicking it only invokes downloadYaml and does not trigger form submission or form handlers.libs/ui-components/src/components/Catalog/CatalogItemDetails.css-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorRemove deprecated
word-break: break-word—overflow-wrap: break-wordon Line 2 already covers this.
word-break: break-wordis listed as deprecated in the CSS specification; it is deprecated in favor ofoverflow-wrap: break-word, which is already declared on Line 2. The property is fully redundant here and Stylelint flags it as an error.🐛 Proposed fix
.fctl-catalog-item-details { overflow-wrap: break-word; - word-break: break-word; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogItemDetails.css` at line 3, Remove the deprecated redundant CSS property in CatalogItemDetails.css by deleting the "word-break: break-word" declaration (it is redundant because "overflow-wrap: break-word" is already present); update the CatalogItemDetails.css rule block that contains both properties so only "overflow-wrap: break-word" remains, ensuring Stylelint no longer flags the deprecated property.libs/ui-components/src/components/Catalog/CatalogItemCard.tsx-41-44 (1)
41-44:⚠️ Potential issue | 🟡 MinorLocalize the image alt text.
Line 43 hardcodes the
iconsuffix, so assistive text stays English-only.💡 Proposed fix
- alt={`${catalogItem.metadata.name} icon`} + alt={t('{{name}} icon', { name: catalogItem.metadata.name })}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogItemCard.tsx` around lines 41 - 44, The alt text for the image in CatalogItemCard currently appends the hardcoded English word "icon" to catalogItem.metadata.name; update this to use the app's localization utility (e.g., t/i18n.translate) so the suffix is localized. Locate the image in CatalogItemCard that calls getCatalogItemIcon(catalogItem) and replace the literal `${catalogItem.metadata.name} icon` with a localized string that accepts the item name as a variable (for example using a translation key like "catalog.itemIcon" or similar) so assistive text is properly translated across locales.libs/ui-components/src/components/Catalog/UpdateModal/AppUpdateModal.tsx-185-200 (1)
185-200:⚠️ Potential issue | 🟡 MinorTypo in prop name:
exisingLabels→existingLabelsThe misspelling appears three times: in the
AppUpdateModalPropstype (Line 185), in the destructured props (Line 197), and at the call site (Line 200). This is a public-facing API surface and should be corrected before the component is consumed externally.🐛 Proposed fix
- exisingLabels: Record<string, string> | undefined; + existingLabels: Record<string, string> | undefined; }; const AppUpdateModal: React.FC<AppUpdateModalProps> = ({ ... - exisingLabels, + existingLabels, }) => { ... - const appConfig = getInitialAppConfig(catalogItem, currentVersion.version, appSpec, exisingLabels); + const appConfig = getInitialAppConfig(catalogItem, currentVersion.version, appSpec, existingLabels);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/AppUpdateModal.tsx` around lines 185 - 200, Rename the misspelled prop exisingLabels to existingLabels across the public API: update the AppUpdateModalProps type declaration, the destructured prop in the AppUpdateModal functional component, and the call to getInitialAppConfig (replace exisingLabels with existingLabels) so the prop name is consistent; ensure any references/tests/usage outside this file are updated to the new existingLabels identifier to avoid runtime/typing errors.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx-54-54 (1)
54-54:⚠️ Potential issue | 🟡 MinorTypo:
exisingLabels→existingLabels.This typo is consistent across the prop definition (Line 54), destructuring (Line 63), and all usage sites (Lines 90, 399, 444). Worth fixing now before it becomes part of the public API surface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` at line 54, Rename the misspelled prop and all its references: change the prop name exisingLabels to existingLabels in the InstalledSoftware component (update the prop type declaration, the props destructuring in the component, and every usage site currently referencing exisingLabels). Ensure you update any exported prop types or interfaces and adjust any consumers/tests that import or pass exisingLabels so they use existingLabels instead to avoid a breaking API change.libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx-33-33 (1)
33-33:⚠️ Potential issue | 🟡 MinorLocal component
CatalogItemListshadows the imported type of the same name.Biome correctly flags this redeclaration. The imported
CatalogItemListtype (Line 33) is only used in thepaginationprop typing (Line 120). Rename the local component or use a type-only import to avoid the collision.Option: rename the local component
-const CatalogItemList = ({ +const CatalogItemListView = ({And update the reference in
SelectAssetModal:- <CatalogItemList + <CatalogItemListViewAlso applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` at line 33, The local React component CatalogItemList is shadowing the imported type CatalogItemList; change one to avoid the collision—either import the type as a type-only import (e.g., "import type { CatalogItemList } from '@flightctl/types/alpha'") so the name remains for the type, or rename the local component (e.g., CatalogItemList -> CatalogItemListView) and update all usages such as the pagination prop typing (pagination) and the reference inside SelectAssetModal to the new component name.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx-348-365 (1)
348-365:⚠️ Potential issue | 🟡 MinorEmpty object
{}pushed into actions array whenitemVersionis falsy.Line 360 pushes
{}(notitle, noonClick) as an action item, which may render as a blank entry in PatternFly'sActionsColumn.🐛 Proposed fix — filter out empty actions
- const actions: IAction[] = [ - itemVersion - ? { - title: t('Edit'), - onClick: () => - setAppToEdit({ - item: app.item, - name: app.name, - channel: appChannel, - version: itemVersion, - }), - } - : {}, - { - title: t('Delete'), - onClick: () => setAppToDelete(app.name), - }, - ]; + const actions: IAction[] = [ + ...(itemVersion + ? [ + { + title: t('Edit'), + onClick: () => + setAppToEdit({ + item: app.item, + name: app.name, + channel: appChannel, + version: itemVersion, + }), + }, + ] + : []), + { + title: t('Delete'), + onClick: () => setAppToDelete(app.name), + }, + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 348 - 365, The actions array currently pushes an empty object when itemVersion is falsy, causing a blank action entry; update the code that builds actions (the const actions: IAction[] block) to only include the Edit action when itemVersion is truthy (i.e., avoid pushing {}). Concretely, replace the ternary that yields {} with a conditional inclusion/filter step (or build the array with a conditional push) so that only valid IAction objects with title and onClick (using setAppToEdit) are present, and ensure the Delete action (using setAppToDelete) remains. Also verify the resulting actions array contains only objects matching IAction before passing to ActionsColumn.libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx-268-273 (1)
268-273:⚠️ Potential issue | 🟡 MinorNo guard for empty
channelsarray on the selected version.If
latestVersion.channelsis an empty array,channels[0]isundefined, violating thestringreturn type and propagating an unexpected value into Formik.♻️ Suggested fix
return { version: latestVersion.version, - channel: latestVersion.channels[0], + channel: latestVersion.channels[0] || '', };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx` around lines 268 - 273, The selected latestVersion (found via versions.find using semver.prerelease) may have an empty channels array causing channel: latestVersion.channels[0] to be undefined; update the logic in the block that computes latestVersion and the returned object (the function that returns { version, channel }) to guard against empty channels by falling back to a safe default string (e.g., '' or 'default') or the first channel from another suitable version: ensure you check latestVersion.channels?.length > 0 before using [0], and if empty use a fallback value so the returned channel is always a string and Formik receives a valid value.libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx-144-152 (1)
144-152:⚠️ Potential issue | 🟡 Minor
Alertnot wrapped inStackItem, unlike the sibling branch.The
<Alert>is a direct child of<Stack>while the deploy<Button>branch is wrapped in<StackItem>. This will cause inconsistent spacing fromhasGutter. Wrap the Alert:♻️ Suggested fix
{item.spec.type === CatalogItemType.CatalogItemTypeData ? ( - <Alert variant="info" isInline title={t('Data catalog item can be deployed as part of an application.')} /> + <StackItem> + <Alert variant="info" isInline title={t('Data catalog item can be deployed as part of an application.')} /> + </StackItem> ) : (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx` around lines 144 - 152, In CatalogItemDetails, the Alert branch is not wrapped in a StackItem which causes inconsistent spacing compared to the Button branch; update the conditional so the Alert (when item.spec.type === CatalogItemType.CatalogItemTypeData) is wrapped in a <StackItem> just like the Deploy branch, ensuring the Alert is a child of StackItem (keep the existing Alert props and text), leaving the Button branch and its onClick/disabled logic (submitForm, installEnabled, isValid) unchanged.libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx-58-61 (1)
58-61:⚠️ Potential issue | 🟡 Minor
forEachcallback should not return a value.The arrow expression body
(c) => (acc[c] = c)implicitly returns the assignment result. Use a block body instead. Also consider using a more idiomatic approach:♻️ Suggested fix
- const channels = catalogItem.spec.versions.reduce((acc, v) => { - v.channels.forEach((c) => (acc[c] = c)); - return acc; - }, {}); + const channels = catalogItem.spec.versions.reduce<Record<string, string>>((acc, v) => { + v.channels.forEach((c) => { acc[c] = c; }); + return acc; + }, {});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx` around lines 58 - 61, The forEach callback inside the catalogItem.spec.versions.reduce is using an arrow expression that implicitly returns the assignment (v.channels.forEach((c) => (acc[c] = c))), which is incorrect; change the callback to a block form that performs the assignment without returning (e.g. v.channels.forEach(c => { acc[c] = c; })), or use a more idiomatic approach such as iterating with for...of or using reduce/flatMap to build the channels object; update the expression that sets the channels constant (the reduce over catalogItem.spec.versions) accordingly so no value is returned from the forEach callback.
libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx (1)
35-52:isUpdatingreset on early-return path — resolved ✅
setIsUpdating(false)at Line 41 now correctly resets the state before the early return, and thecatch/finallyblock handles API errors. Both previously flagged issues are addressed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx` around lines 35 - 52, The early reset of isUpdating in handleUpdate is redundant now that the try/catch/finally always calls setIsUpdating(false); remove the inline setIsUpdating(false) before the early return (after checking catalogItemVersion via catalogItem.spec.versions.find with selectedVersion) and rely on the finally block to clear isUpdating so the state is reset exactly once on all exit paths.
🧹 Nitpick comments (1)
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
219-220:useMemoempty-dep array is inconsistent withenableReinitialize.
initialValuesis memoised withoutitemin the dependency array (theeslint-disablesuppresses the warning), whileenableReinitializeon theFormikwrapper exists precisely to react toinitialValueschanges. In practiceCatalogItemDetailsis always remounted on a new selection, so there is no runtime bug today, but the combination is misleading:enableReinitializeis effectively dead, and if the parent ever optimises to keep the component mounted while swappingitem, the form will not reinitialise.Either include
itemin the dependency array and remove the eslint-disable, or removeenableReinitializeto document the real intent:♻️ Proposed fix (option A — keep recomputation when item changes)
- // eslint-disable-next-line react-hooks/exhaustive-deps - const initialValues = React.useMemo(() => getDefaultChannelAndVersion(item), []); + const initialValues = React.useMemo(() => getDefaultChannelAndVersion(item), [item]);♻️ Proposed fix (option B — document that remount is the reset mechanism)
- <Formik<InstallSpecFormik> initialValues={initialValues} enableReinitialize onSubmit={onSubmit}> + <Formik<InstallSpecFormik> initialValues={initialValues} onSubmit={onSubmit}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 219 - 220, initialValues is memoized via React.useMemo(() => getDefaultChannelAndVersion(item), []) while Formik is using enableReinitialize, which is inconsistent because the memo never updates when item changes; either include item in the dependency array (remove the // eslint-disable-next-line react-hooks/exhaustive-deps) so initialValues recomputes when item changes, or remove enableReinitialize from the Formik wrapper to document that the component relies on remount to reset the form—update the useMemo call referencing getDefaultChannelAndVersion and the eslint-disable accordingly, or remove enableReinitialize from the Formik props to make the intent explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx`:
- Around line 112-119: The schema currently only checks presence for
channel/version; update validationSchema (the Yup.lazy for InstallOsFormik) to
validate allowed values instead of just required: for channel use
Yup.string().oneOf(availableChannelIds, t('Invalid channel
selected')).required(...), and for version compute the allowed version IDs for
the selected channel (e.g., derive allowedVersionIds from availableVersions or
from availableChannels[values.channel]) and use
Yup.string().oneOf(allowedVersionIds, t('Invalid version
selected')).required(...); ensure this uses the same
availableChannels/availableVersions data your component provides so the form
will surface errors instead of hitting the silent return path in the submit
handler.
- Around line 141-146: The code uses optional chaining when building resourceId
(selectedDevice?.metadata.name and selectedFleet?.metadata.name); instead
enforce that a selection exists and read metadata.name directly. Update the
logic around values.device / values.fleet (selectedDevice and selectedFleet) and
the installToDevice check to validate/ensure the chosen resource is present,
then construct resourceId using selectedDevice.metadata.name or
selectedFleet.metadata.name without ?. so the code relies on guaranteed resource
shapes (e.g., after validation or a guard).
In `@libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx`:
- Line 57: The Modal's onClose must be guarded while an update is in flight:
wrap the existing onClose so it no-ops when isUpdating is true (e.g., replace
onClose={onClose} with a guarded handler that calls onClose() only if
!isUpdating) and also disable the header close / Escape-key paths while updating
by setting the Modal props that control those actions (e.g., disable Escape and
hide/disable the modal close button via the Modal props used in this codebase)
so handleUpdate can run to completion without the modal being dismissed.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 303-304: Remove the unnecessary fallback by replacing the
expression that sets assetItemName (currently using item.metadata.name || '') to
use item.metadata.name directly; since item is a CatalogItem returned from the
API, metadata.name is guaranteed defined, so update the assignment in the
VolumeImageField component (the code that sets assetCatalog and assetItemName)
to use item.metadata.name without the || '' fallback and ensure any related
locals/props expecting a possibly-empty string accept the defined name type.
- Line 33: The local component named CatalogItemList is shadowing the imported
CatalogItemList type; rename the local component (e.g. to AssetCatalogList or
CatalogItemListPanel) and update all references to it (component declaration,
any exports, and where SelectAssetModal renders or references CatalogItemList)
so the imported type PaginationDetails<CatalogItemList> continues to refer to
the type and the linter noRedeclare error is resolved; ensure the component name
change is applied consistently across props, JSX usages, and any default export.
---
Duplicate comments:
In `@libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx`:
- Around line 35-52: The early reset of isUpdating in handleUpdate is redundant
now that the try/catch/finally always calls setIsUpdating(false); remove the
inline setIsUpdating(false) before the early return (after checking
catalogItemVersion via catalogItem.spec.versions.find with selectedVersion) and
rely on the finally block to clear isUpdating so the state is reset exactly once
on all exit paths.
---
Nitpick comments:
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 219-220: initialValues is memoized via React.useMemo(() =>
getDefaultChannelAndVersion(item), []) while Formik is using enableReinitialize,
which is inconsistent because the memo never updates when item changes; either
include item in the dependency array (remove the // eslint-disable-next-line
react-hooks/exhaustive-deps) so initialValues recomputes when item changes, or
remove enableReinitialize from the Formik wrapper to document that the component
relies on remount to reset the form—update the useMemo call referencing
getDefaultChannelAndVersion and the eslint-disable accordingly, or remove
enableReinitialize from the Formik props to make the intent explicit.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/ui-components/src/components/Catalog/CatalogItemCard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsxlibs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsxlibs/ui-components/src/components/Catalog/utils.tslibs/ui-components/src/components/DynamicForm/VolumeImageField.tsxlibs/ui-components/src/components/form/LabelsField.tsxlibs/ui-components/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (4)
- libs/ui-components/tsconfig.json
- libs/ui-components/src/components/form/LabelsField.tsx
- libs/ui-components/src/components/Catalog/utils.ts
- libs/ui-components/src/components/Catalog/CatalogItemCard.tsx
| const validationSchema = Yup.lazy((values: InstallOsFormik) => | ||
| Yup.object({ | ||
| target: Yup.string().required(t('Target must be selected')), | ||
| device: values.target === 'device' ? Yup.object().required(t('Device must be selected')) : Yup.object(), | ||
| fleet: values.target === 'fleet' ? Yup.object().required(t('Fleet must be selected')) : Yup.object(), | ||
| channel: Yup.string().required(t('Channel must be selected')), | ||
| version: Yup.string().required(t('Version must be selected')), | ||
| }), |
There was a problem hiding this comment.
Schema should validate allowed channel/version values, not just presence.
On Line 117 and Line 118, required() alone allows stale/tampered URL values. Downstream, that can hit the silent return path on Line 152-Line 154 with no user feedback.
💡 Proposed fix
+ const allowedChannels = catalogItem.spec.channels.map((c) => c.name);
+ const allowedVersions = catalogItem.spec.versions.map((v) => v.version);
+
const validationSchema = Yup.lazy((values: InstallOsFormik) =>
Yup.object({
target: Yup.string().required(t('Target must be selected')),
device: values.target === 'device' ? Yup.object().required(t('Device must be selected')) : Yup.object(),
fleet: values.target === 'fleet' ? Yup.object().required(t('Fleet must be selected')) : Yup.object(),
- channel: Yup.string().required(t('Channel must be selected')),
- version: Yup.string().required(t('Version must be selected')),
+ channel: Yup.string()
+ .oneOf(allowedChannels, t('Channel must be selected'))
+ .required(t('Channel must be selected')),
+ version: Yup.string()
+ .oneOf(allowedVersions, t('Version must be selected'))
+ .required(t('Version must be selected')),
}),
);Also applies to: 150-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx`
around lines 112 - 119, The schema currently only checks presence for
channel/version; update validationSchema (the Yup.lazy for InstallOsFormik) to
validate allowed values instead of just required: for channel use
Yup.string().oneOf(availableChannelIds, t('Invalid channel
selected')).required(...), and for version compute the allowed version IDs for
the selected channel (e.g., derive allowedVersionIds from availableVersions or
from availableChannels[values.channel]) and use
Yup.string().oneOf(allowedVersionIds, t('Invalid version
selected')).required(...); ensure this uses the same
availableChannels/availableVersions data your component provides so the form
will surface errors instead of hitting the silent return path in the submit
handler.
| const selectedDevice = values.device; | ||
| const selectedFleet = values.fleet; | ||
| const installToDevice = values.target === 'device'; | ||
| const resourceId = installToDevice | ||
| ? `devices/${selectedDevice?.metadata.name}` | ||
| : `fleets/${selectedFleet?.metadata.name}`; |
There was a problem hiding this comment.
Avoid optional chaining when reading metadata.name for selected resources.
Line 145 and Line 146 should not use ?. for metadata.name in this flow; enforce selection first, then use direct metadata.name.
💡 Proposed fix
const selectedDevice = values.device;
const selectedFleet = values.fleet;
const installToDevice = values.target === 'device';
+ if (installToDevice && !selectedDevice) {
+ setError(t('Device must be selected'));
+ return;
+ }
+ if (!installToDevice && !selectedFleet) {
+ setError(t('Fleet must be selected'));
+ return;
+ }
const resourceId = installToDevice
- ? `devices/${selectedDevice?.metadata.name}`
- : `fleets/${selectedFleet?.metadata.name}`;
+ ? `devices/${selectedDevice.metadata.name}`
+ : `fleets/${selectedFleet.metadata.name}`;As per coding guidelines, "Treat resource.metadata.name as always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const selectedDevice = values.device; | |
| const selectedFleet = values.fleet; | |
| const installToDevice = values.target === 'device'; | |
| const resourceId = installToDevice | |
| ? `devices/${selectedDevice?.metadata.name}` | |
| : `fleets/${selectedFleet?.metadata.name}`; | |
| const selectedDevice = values.device; | |
| const selectedFleet = values.fleet; | |
| const installToDevice = values.target === 'device'; | |
| if (installToDevice && !selectedDevice) { | |
| setError(t('Device must be selected')); | |
| return; | |
| } | |
| if (!installToDevice && !selectedFleet) { | |
| setError(t('Fleet must be selected')); | |
| return; | |
| } | |
| const resourceId = installToDevice | |
| ? `devices/${selectedDevice.metadata.name}` | |
| : `fleets/${selectedFleet.metadata.name}`; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx`
around lines 141 - 146, The code uses optional chaining when building resourceId
(selectedDevice?.metadata.name and selectedFleet?.metadata.name); instead
enforce that a selection exists and read metadata.name directly. Update the
logic around values.device / values.fleet (selectedDevice and selectedFleet) and
the installToDevice check to validate/ensure the chosen resource is present,
then construct resourceId using selectedDevice.metadata.name or
selectedFleet.metadata.name without ?. so the code relies on guaranteed resource
shapes (e.g., after validation or a guard).
libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx
Outdated
Show resolved
Hide resolved
| assetCatalog: item.metadata.catalog, | ||
| assetItemName: item.metadata.name || '', |
There was a problem hiding this comment.
Drop the || '' fallback on item.metadata.name.
item here is a CatalogItem returned from the API, so metadata.name must be treated as always defined per the project coding guideline. Using || '' implies it could be undefined, which contradicts the guideline.
♻️ Proposed fix
- assetItemName: item.metadata.name || '',
+ assetItemName: item.metadata.name,As per coding guidelines, "Treat resource.metadata.name as always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assetCatalog: item.metadata.catalog, | |
| assetItemName: item.metadata.name || '', | |
| assetCatalog: item.metadata.catalog, | |
| assetItemName: item.metadata.name, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around
lines 303 - 304, Remove the unnecessary fallback by replacing the expression
that sets assetItemName (currently using item.metadata.name || '') to use
item.metadata.name directly; since item is a CatalogItem returned from the API,
metadata.name is guaranteed defined, so update the assignment in the
VolumeImageField component (the code that sets assetCatalog and assetItemName)
to use item.metadata.name without the || '' fallback and ensure any related
locals/props expecting a possibly-empty string accept the defined name type.
There was a problem hiding this comment.
Actionable comments posted: 11
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx (1)
153-159:⚠️ Potential issue | 🟡 Minor
'catalog'intabKeysbut the tab/route only exist whenisEnrolled— direct URL navigation breaks.
tabKeysunconditionally includes'catalog', while the<Tab>and<Route path="catalog">are both gated onisEnrolled. If a user directly navigates to/devices/:id/catalogon a non-enrolled device, there is no matching<Route>, so nothing renders (no redirect either). Consider either removing'catalog'fromtabKeyswhen not enrolled, or adding a fallback<Route path="catalog" element={<Navigate to="details" replace />} />for that case.🐛 Proposed fix — guard tabKeys
-<TabsNav aria-label="Device details tabs" tabKeys={['details', 'catalog', 'yaml', 'terminal', 'events']}> +<TabsNav aria-label="Device details tabs" tabKeys={['details', ...(isEnrolled ? ['catalog'] : []), 'yaml', 'terminal', 'events']}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx` around lines 153 - 159, The tabs list in DeviceDetailsPage includes 'catalog' unconditionally in the TabsNav tabKeys while the actual <Tab eventKey="catalog"> and the corresponding Route are only rendered when isEnrolled, causing broken direct navigation; update the tabKeys construction so it conditionally includes 'catalog' only when isEnrolled (e.g., build tabKeys from an array that pushes 'catalog' if isEnrolled) or alternatively add a fallback Route for path="catalog" that redirects to "details" when isEnrolled is false; locate the TabsNav/tabKeys usage and the Route for "catalog" in DeviceDetailsPage to implement the chosen fix.libs/types/scripts/openapi-typescript.js (2)
126-131:⚠️ Potential issue | 🟡 MinorMissing cleanup of
alphaandtmp-alpha-typesdirectories.The initial cleanup in
main()removesmodels,imagebuilder,tmp-types, andtmp-imagebuilder-types, but doesn't cleanalphaortmp-alpha-types. This could leave stale alpha type files from a previous generation run.await Promise.all([ rimraf(path.join(rootDir, 'models')), rimraf(path.join(rootDir, 'imagebuilder')), + rimraf(path.join(rootDir, 'alpha')), rimraf(path.join(rootDir, 'tmp-types')), rimraf(path.join(rootDir, 'tmp-imagebuilder-types')), + rimraf(path.join(rootDir, 'tmp-alpha-types')), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 126 - 131, The cleanup in main() currently calls rimraf for 'models', 'imagebuilder', 'tmp-types', and 'tmp-imagebuilder-types' but misses the 'alpha' and 'tmp-alpha-types' directories; update the Promise.all cleanup array in openapi-typescript.js to also rimraf path.join(rootDir, 'alpha') and path.join(rootDir, 'tmp-alpha-types') so stale alpha type files are removed on each run (look for the rimraf calls in the initial cleanup block inside main()).
52-52:⚠️ Potential issue | 🟡 MinorStale error message — missing
alphacoremode.The error message still lists only
'core' or 'imagebuilder'but'alphacore'is now a valid mode.- throw new Error(`Unknown mode: ${mode}. Use 'core' or 'imagebuilder'`); + throw new Error(`Unknown mode: ${mode}. Use 'core', 'alphacore', or 'imagebuilder'`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` at line 52, Update the thrown Error in openapi-typescript.js that currently reads throw new Error(`Unknown mode: ${mode}. Use 'core' or 'imagebuilder'`) to include the newly supported 'alphacore' option; locate the throw that uses the mode variable and change the message to list 'core', 'imagebuilder', and 'alphacore' (or construct the allowed-modes string dynamically) so the error accurately reflects valid modes.libs/ui-components/src/components/Device/DevicesPage/useDevices.ts (1)
55-65:⚠️ Potential issue | 🟡 MinorAvoid building contradictory owner selectors.
If
ownerFleetsis set andonlyFleetlessis true, the query can include bothmetadata.owner=...and!metadata.owner, which collapses to no results.🔧 Proposed guard
- if (ownerFleets?.length) { + if (onlyFleetless) { + fieldSelectors.push('!metadata.owner'); + } else if (ownerFleets?.length) { queryUtils.addQueryConditions( fieldSelectors, 'metadata.owner', ownerFleets.map((fleet) => `Fleet/${fleet}`), ); } - - if (onlyFleetless) { - fieldSelectors.push('!metadata.owner'); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Device/DevicesPage/useDevices.ts` around lines 55 - 65, The code can push contradictory owner selectors when both ownerFleets and onlyFleetless are set; update the logic so only one wins: either skip adding metadata.owner conditions if onlyFleetless is true, or skip the onlyFleetless push when ownerFleets exist. Concretely, modify the block around queryUtils.addQueryConditions (and the following onlyFleetless push) to check onlyFleetless before adding ownerFleets (e.g., if (!onlyFleetless && ownerFleets?.length) { queryUtils.addQueryConditions(...); } else if (onlyFleetless) { fieldSelectors.push('!metadata.owner'); }) so you never add both selectors.libs/types/package.json (1)
26-30:⚠️ Potential issue | 🟠 Major
"alpha"is missing from the"files"array — the directory won't be included in the published package.
"imagebuilder"is explicitly listed so its source ships alongsidedist, but"alpha"is absent. Any consumer of the published@flightctl/typespackage will get a broken./alphaexport even though it resolves fine in the monorepo.🐛 Proposed fix
"files": [ "dist", "index.ts", - "imagebuilder" + "imagebuilder", + "alpha" ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/package.json` around lines 26 - 30, The package.json "files" array for the `@flightctl/types` package is missing the "alpha" directory so that folder won't be included in the published package; update the "files" array in libs/types/package.json to add "alpha" alongside "dist" and "imagebuilder" so the ./alpha export is shipped with the package and consumers won't get a broken import.
♻️ Duplicate comments (13)
libs/types/alpha/models/CatalogItemMeta.ts (1)
1-14: Codegen tool discrepancy already flagged inCatalogItemDeprecation.tsapplies here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/alpha/models/CatalogItemMeta.ts` around lines 1 - 14, This file has the same codegen-tool discrepancy called out in CatalogItemDeprecation.ts; apply the identical fix used there to CatalogItemMeta: update the generated type definition for CatalogItemMeta (the ObjectMeta import and the catalog property) to match the corrected shape applied in CatalogItemDeprecation.ts (e.g., optional vs required catalog, naming/typing changes), ensuring the exported CatalogItemMeta type mirrors that corrected pattern and remove any duplicate/incorrect generated artifacts.libs/types/alpha/models/ApiVersion.ts (1)
1-11: Codegen tool discrepancy already flagged inCatalogItemDeprecation.tsapplies here too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/alpha/models/ApiVersion.ts` around lines 1 - 11, The ApiVersion enum here was generated with a different codegen configuration than other models (same discrepancy noted for CatalogItemDeprecation); fix it by regenerating models with the same openapi-typescript-codegen settings used for CatalogItemDeprecation or manually align the ApiVersion enum in ApiVersion.ts so its member names and string literals exactly match the canonical representation used elsewhere (refer to the ApiVersion enum symbol and the CatalogItemDeprecation usage to ensure identical identifiers/values).libs/types/scripts/openapi-typescript.js (1)
77-96: Copy-paste comments still reference "imagebuilder" instead of "alpha".Lines 78, 87, and 90 contain comments and variable names (
imagebuilderDir) that reference "imagebuilder" but this block processes the alpha API types.Suggested fix
} else if (mode === ALPHA_CORE_API) { - // Image builder types need to be fixed before they can be moved to their final location + // Alpha types need to be fixed before they can be moved to their final location await rimraf(finalDir); const modelsDir = path.join(output, 'models'); if (fs.existsSync(modelsDir)) { @@ -87,9 +87,9 @@ await fixCoreReferences(finalDir); - // Copy the generated index.ts to imagebuilder/index.ts + // Copy the generated index.ts to alpha/index.ts const indexPath = path.join(output, 'index.ts'); if (fs.existsSync(indexPath)) { - const imagebuilderDir = path.resolve(__dirname, '../alpha'); - if (!fs.existsSync(imagebuilderDir)) { - fs.mkdirSync(imagebuilderDir, { recursive: true }); + const alphaDir = path.resolve(__dirname, '../alpha'); + if (!fs.existsSync(alphaDir)) { + fs.mkdirSync(alphaDir, { recursive: true }); } - await fsPromises.copyFile(indexPath, path.join(imagebuilderDir, 'index.ts')); + await fsPromises.copyFile(indexPath, path.join(alphaDir, 'index.ts')); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 77 - 96, The block handling ALPHA_CORE_API still uses "imagebuilder" in comments and the variable imagebuilderDir; rename comments and variables to reference "alpha" instead (e.g., imagebuilderDir -> alphaDir) and update the comment on copying the generated index.ts to say "Copy the generated index.ts to alpha/index.ts"; update any related fsPromises.copyFile destination to use the new alphaDir and ensure references to "imagebuilder" in the surrounding log messages (like the console.log and any doc comments) are changed to "alpha" while keeping the existing use of finalDir, modelsDir, fixCoreReferences, and indexPath intact.apps/ocp-plugin/console-extensions.json (1)
38-47: "Software Catalog" naming — confirmed intentional.Per prior review discussion, "Software Catalog" is the agreed naming. No action needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ocp-plugin/console-extensions.json` around lines 38 - 47, The "Software Catalog" label in the console-extensions entry is intentional; no code changes are required—leave the object with id "fctl-catalog" and the name property "%plugin__flightctl-plugin~Software Catalog%" (and its href "/edge/catalog", perspective "acm", section "fctl") as-is.libs/ui-components/src/components/Catalog/CatalogPage.tsx (1)
208-208:filterIsEmptyis alwaysfalse— empty-state logic is broken.
catalogFilter.itemTypeis an array, and!arrayis alwaysfalsein JavaScript (arrays are truthy, even when empty). This means the "No catalog items yet" empty state is never shown; it always shows "No results found".- const filterIsEmpty = !catalogFilter.itemType; + const filterIsEmpty = catalogFilter.itemType.length === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx` at line 208, filterIsEmpty is computed incorrectly because catalogFilter.itemType is an array (which is always truthy); update the logic in CatalogPage so it checks the array length instead of negating the array. For example, replace the current const filterIsEmpty = !catalogFilter.itemType with a check that handles arrays safely, e.g. use Array.isArray(catalogFilter.itemType) ? catalogFilter.itemType.length === 0 : !catalogFilter.itemType (referencing catalogFilter.itemType and filterIsEmpty in the CatalogPage component).libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx (1)
57-57: ModalonCloseis not guarded during an in-flight update.The
×button andEscapekey can still dismiss the modal whilehandleUpdateis running, sinceonCloseis passed unconditionally. Consider guarding it:-<Modal isOpen onClose={onClose} variant="large"> +<Modal isOpen onClose={isUpdating ? undefined : onClose} variant="large">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx` at line 57, The Modal's onClose is passed unconditionally which allows closing while handleUpdate is running; guard the close action by checking the update state (e.g., isUpdating or similar) before calling onClose: update the Modal instantiation that currently passes onClose to only pass it (or enable closing) when not in-flight, or alternatively wire Modal props (escape/overlay/close button) to a flag derived from isUpdating so handleUpdate cannot be interrupted; reference the Modal component and the handleUpdate/isUpdating state to implement this guard.libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx (1)
42-45: DuplicateInstallSpecFormiktype — also defined intypes.ts.This duplicates the type from
types.ts. Import it instead to keep a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx` around lines 42 - 45, Remove the duplicate InstallSpecFormik type declaration in SpecificationsStep.tsx and import the canonical InstallSpecFormik from the existing types.ts instead; specifically delete the local "export type InstallSpecFormik = { version: string; channel: string }" and add an import for InstallSpecFormik at the top of the file, then ensure all references in SpecificationsStep (e.g., any type annotations or props using InstallSpecFormik) use the imported symbol.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx (1)
54-55:⚠️ Potential issue | 🟡 MinorSame
exisingLabelstypo as inAppUpdateModal.tsx.This typo is consistent across the component tree (
UpdateColumnProps,UpdateAppColumn,InstalledSoftware). Fix in tandem withAppUpdateModal.Also applies to: 63-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 54 - 55, Rename the misspelled prop/type "exisingLabels" to "existingLabels" everywhere it appears—update the declaration in InstalledSoftware (type/props), the UpdateColumnProps interface, the UpdateAppColumn component, and any usages/imports (including AppUpdateModal.tsx) so names stay consistent; ensure TypeScript types and prop references are updated together and run tests/TS compile to catch any remaining references.libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx (2)
144-146:⚠️ Potential issue | 🟠 MajorOptional chaining on
metadata.namestill present — prior review comment unresolved.Lines 145–146 still use
selectedDevice?.metadata.nameandselectedFleet?.metadata.name. Per the prior review and coding guidelines, add a guard for the undefined case and then access.metadata.namedirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx` around lines 144 - 146, resourceId construction is still using optional chaining on selectedDevice?.metadata.name and selectedFleet?.metadata.name; update the logic so you first guard that the selected object exists then access metadata.name directly (e.g., check installToDevice ? ensure selectedDevice is defined and read selectedDevice.metadata.name : ensure selectedFleet is defined and read selectedFleet.metadata.name), and handle the undefined case explicitly (throw/error log or provide a clear fallback) so resourceId is built from a guaranteed .metadata.name rather than using optional chaining.
150-154:⚠️ Potential issue | 🟡 MinorSilent return when
catalogItemVersionis not found orchannelis empty.Unlike the error paths in the
catchblock (Line 173–175), Lines 152–154 return silently with no user feedback. The wizard stays on the review step in an indeterminate state.💡 Proposed fix
if (!catalogItemVersion || !values.channel) { - return; + setError(t('Version or channel is not valid')); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsx` around lines 150 - 154, The code currently returns silently when catalogItemVersion is missing or values.channel is empty inside the InstallOsWizard submission flow (check around catalogItemVersion and values.channel), leaving the wizard stuck; update this branch to surface an error to the user instead of returning: set a visible error state or call the form error/notification handler (e.g., setFieldError or dispatch a toast) with a clear message, ensure any submitting/loading flags are cleared (e.g., stop spinner) and abort further processing; reference the catalogItemVersion check and values.channel validation in InstallOsWizard so reviewers can find and replace the silent return with the error/cleanup logic.libs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsx (1)
167-172:⚠️ Potential issue | 🟠 MajorGuard against undefined device/fleet before building
resourceId.If
selectedDeviceorselectedFleetisundefined(e.g., due to a race or unexpected state),resourceIdbecomes"devices/undefined", leading to a bogus API call. Add an explicit guard, consistent with the pattern already flagged inInstallOsWizard.💡 Proposed fix
+ if (installToDevice && !selectedDevice) { + return; + } + if (!installToDevice && !selectedFleet) { + return; + } const resourceId = installToDevice - ? `devices/${selectedDevice?.metadata.name}` - : `fleets/${selectedFleet?.metadata.name}`; + ? `devices/${selectedDevice.metadata.name}` + : `fleets/${selectedFleet.metadata.name}`;As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsx` around lines 167 - 172, The resourceId construction in InstallAppWizard uses selectedDevice/selectedFleet without guarding for undefined, which can produce "devices/undefined" — add an explicit guard before building resourceId: check values.device when installToDevice is true and values.fleet when false (use the same pattern as InstallOsWizard), and if the required resource is undefined, return early or set an error/validation state instead of constructing the ID; update references to installToDevice, selectedDevice, selectedFleet, and resourceId accordingly so the API call never receives a bogus resource string.libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (2)
304-304:⚠️ Potential issue | 🟡 MinorRemove the fallback from
item.metadata.name.
itemis API data, sometadata.nameshould be treated as defined here.♻️ Proposed fix
- assetItemName: item.metadata.name || '', + assetItemName: item.metadata.name,As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` at line 304, In the VolumeImageField component replace the fallback usage "item.metadata.name || ''" with the guaranteed value "item.metadata.name" (remove the empty-string fallback and any optional chaining) so assetItemName uses the API-provided name directly; if TypeScript complains adjust the value's type where assetItemName is declared (or add a non-null assertion) to reflect that resource.metadata.name is always defined.
33-33:⚠️ Potential issue | 🟠 MajorResolve the
CatalogItemListidentifier collision.The imported
CatalogItemListsymbol and localconst CatalogItemListcollide in module scope, which is a redeclaration/lint blocker. Rename one side (or alias/type-import the model type).♻️ Proposed fix
-import { CatalogItem, CatalogItemList, CatalogItemType, CatalogItemVersion } from '@flightctl/types/alpha'; +import { CatalogItem, CatalogItemType, CatalogItemVersion, type CatalogItemList } from '@flightctl/types/alpha'; ... -const CatalogItemList = ({ +const AssetCatalogList = ({ assetCatalogItems, isLoading, isUpdating, error, pagination, onSelect, onClose, nameFilter, setNameFilter, }: CatalogItemListProps) => { ... - <CatalogItemList + <AssetCatalogList onSelect={setSelectedAsset} onClose={onClose} assetCatalogItems={assetCatalogItems}#!/bin/bash # Verify the conflicting identifier usage in this module. rg -n -C2 --type=tsx '\bCatalogItemList\b' libs/ui-components/src/components/DynamicForm/VolumeImageField.tsxAlso applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` at line 33, The module imports CatalogItemList and also declares a local const named CatalogItemList causing a redeclaration; fix by renaming one side and updating usages — either alias the imported type (e.g., import { CatalogItemList as CatalogItemModel } from '@flightctl/types/alpha' or import it as a type-only import) or rename the local const (e.g., CatalogItemListOptions or CatalogItemListComponent) and update every reference in this file (including where the local const is used) so the identifiers no longer collide.
🟡 Minor comments (18)
libs/ui-components/src/components/Fleet/useFleets.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 Minor
limitis exposed but never applied to the backend request.Line 18 adds
limit?: number, butgetFleetsEndpointstill always sendslimit=${PAGE_SIZE}(Line 59). This makes the new API argument ineffective and misleading for callers.Proposed fix
const getFleetsEndpoint = ({ name, addDevicesSummary, nextContinue, + limit, }: { name?: string; addDevicesSummary?: boolean; nextContinue?: string; + limit?: number; }) => { const params = new URLSearchParams({ - limit: `${PAGE_SIZE}`, + limit: `${limit ?? PAGE_SIZE}`, });Also applies to: 49-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Fleet/useFleets.ts` at line 18, The new optional parameter limit on the hook is never passed to the backend: update getFleetsEndpoint (and any other place building the fetch URL in useFleets.ts) to use the provided limit when present (e.g., use limit ?? PAGE_SIZE) instead of always interpolating PAGE_SIZE; ensure the query string includes limit=<value> and retains PAGE_SIZE as the default when limit is undefined so callers who pass limit get the requested page size.libs/ui-components/src/components/Catalog/UpdateModal/EditOsModal.tsx-22-24 (1)
22-24:⚠️ Potential issue | 🟡 MinorMissing question mark in confirmation text.
The prompt reads as a question but lacks a trailing
?.- {t('Are you sure you want to deploy operating system {{os}}', { + {t('Are you sure you want to deploy operating system {{os}}?', {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/EditOsModal.tsx` around lines 22 - 24, The confirmation string in EditOsModal is missing a trailing question mark; update the call to t(...) inside the JSX (the template string passed to the i18n function in EditOsModal) to include a final "?" so the rendered prompt becomes "Are you sure you want to deploy operating system {{os}}?" while preserving the interpolation of os: item.spec.displayName || item.metadata.name.libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx-153-160 (1)
153-160:⚠️ Potential issue | 🟡 MinorUse
type="button"for the Download button for semantic correctness.Line 155 uses
type="submit"for a download action. Since other action buttons (Save, Reload, Cancel) correctly usetype="button"(the default), the Download button should match for consistency and semantic accuracy.- type="submit" + type="button"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsx` around lines 153 - 160, Change the Download button in YamlEditorBase from type="submit" to type="button" to make the action semantically correct and consistent with the other controls; update the Button JSX that renders the Download icon (the one with onClick={downloadYaml}) so it uses type="button" instead of "submit" to avoid accidental form submission when invoking downloadYaml.libs/ui-components/src/components/Catalog/UpdateModal/EditAppModal.tsx-145-147 (1)
145-147:⚠️ Potential issue | 🟡 MinorAdd optional chaining to guard
appSpecdereference at line 145.While
appSpecis optional and the code is only reached whenselectedAssetsis populated (which only happens whenappSpecis defined), the unsafe type assertion(appSpec as ContainerApplication).volumescreates a latent type-safety issue. Use optional chaining for clarity and robustness:Suggested fix
- const volumes = (appSpec as ContainerApplication).volumes; + const volumes = (appSpec as ContainerApplication | undefined)?.volumes; const volume = volumes ? volumes[volumeIdx] : undefined; - const imgRef = (volume as ImageVolumeProviderSpec).image?.reference; + const imgRef = (volume as ImageVolumeProviderSpec | undefined)?.image?.reference;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/EditAppModal.tsx` around lines 145 - 147, The code unsafely asserts appSpec as a ContainerApplication when reading volumes; change the access to use optional chaining so you don't dereference undefined (e.g., read volumes via (appSpec as ContainerApplication | undefined)?.volumes) and then propagate that safety to subsequent uses (handle volumes possibly undefined before indexing with volumeIdx and when computing imgRef from (volume as ImageVolumeProviderSpec)?.image?.reference). Ensure references to appSpec, ContainerApplication, volumes, volumeIdx, volume, and imgRef are updated so they tolerate undefined values.libs/i18n/locales/en/translation.json-315-315 (1)
315-315:⚠️ Potential issue | 🟡 MinorMissing question mark in user-facing text.
This confirmation prompt reads as a statement rather than a question.
- "Are you sure you want to deploy operating system {{os}}": "Are you sure you want to deploy operating system {{os}}", + "Are you sure you want to deploy operating system {{os}}?": "Are you sure you want to deploy operating system {{os}}?",Note: Fix this in the source code component that uses this key, then regenerate with
npm run i18n.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/i18n/locales/en/translation.json` at line 315, Update the user-facing translation for the i18n key "Are you sure you want to deploy operating system {{os}}" to include a trailing question mark in the source component that uses this key (so the prompt reads "Are you sure you want to deploy operating system {{os}}?"), then regenerate translation files by running npm run i18n; locate usages by searching for the exact key string or its lookup in components that perform OS deployment confirmation (e.g., confirmation dialog or deploy handler) and update the source before running the i18n extraction.libs/ui-components/src/components/common/LabelsView.tsx-14-14 (1)
14-14:⚠️ Potential issue | 🟡 MinorUse
.startsWith()instead of.includes()for prefix matching.
CATALOG_LABELis correctly defined as'catalog.flightctl.io/'(a namespace prefix). However,.includes()performs substring matching, which could theoretically filter unintended labels if they contain this substring elsewhere. Use.startsWith(CATALOG_LABEL)to explicitly and robustly match the namespace prefix:Suggested fix
- const labelItems = Object.entries(labels || {}).filter(([key]) => !key.includes(CATALOG_LABEL)); + const labelItems = Object.entries(labels || {}).filter(([key]) => !key.startsWith(CATALOG_LABEL));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/LabelsView.tsx` at line 14, The filter that builds labelItems currently uses .includes() and may match CATALOG_LABEL as a substring anywhere; update the predicate in the labelItems creation (the Object.entries(...).filter(([key]) => ...) expression) to use key.startsWith(CATALOG_LABEL) instead of key.includes(CATALOG_LABEL) so only keys prefixed with CATALOG_LABEL are excluded.libs/ui-components/src/components/DynamicForm/DynamicForm.tsx-82-85 (1)
82-85:⚠️ Potential issue | 🟡 Minor
onValidatewon't fire until the first user interaction.Since
onValidateis only called insideonChange, the parent won't receive the initial validity state. If the form renders with invalid initial data, the parent won't know until the user makes a change. Consider firing an initial validation check after mount if consumers rely ononValidateto gate navigation (e.g., wizard "Next" button enablement).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/DynamicForm.tsx` around lines 82 - 85, The component only calls onValidate inside the onChange handler so the parent never sees initial validity; add an effect in DynamicForm that runs once after mount to compute initial validity and call onValidate with a boolean. Implement this by creating/using the form instance or a ref (the same form ref used by the onChange handler) to run the form's validation method or compute errors for the initial formData, then call onValidate(initialIsValid) so consumers get the initial validation state even before any user interaction.libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx-120-124 (1)
120-124:⚠️ Potential issue | 🟡 MinorThrowing a translated string instead of an
Errorobject.
throw t('Version {{version}} not found', ...)throws a raw string. Error handlers (e.g.,catch (e) { e.message }) that expect anErrorinstance will fail to extract a message. Usenew Error(...)instead.Suggested fix
- throw t('Version {{version}} not found', { version: installItem.version }); + throw new Error(t('Version {{version}} not found', { version: installItem.version }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx` around lines 120 - 124, The onSubmit handler is throwing a raw translated string (throw t(...)) which should be an Error object; replace the string throw with throwing a new Error containing the translated message so downstream catch blocks can read e.message. Update the onSubmit block where catalogItemVersion is checked (the installItem.item.spec.versions.find(...) branch) to create and throw new Error(t('Version {{version}} not found', { version: installItem.version })) instead of throwing the raw string.libs/ui-components/src/components/Catalog/InstallWizard/InstallWizard.tsx-34-47 (1)
34-47:⚠️ Potential issue | 🟡 MinorNo fallback for unrecognized
category— user sees an empty wizard area.If
catalogItemloads successfully but itsspec.categoryis neitherSystemnorApplication(e.g., a future category type, orundefinedsincecategoryis optional inCatalogItemSpec),contentstaysundefinedand the wizard section renders empty with no feedback.Suggested fallback
} else if (catalogItem?.spec.category === CatalogItemCategory.CatalogItemCategoryApplication) { content = <InstallAppWizard catalogItem={catalogItem} />; + } else if (catalogItem) { + content = ( + <Alert isInline variant="warning" title={t('Unsupported catalog item category')}> + {catalogItem.spec.category} + </Alert> + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallWizard.tsx` around lines 34 - 47, The current branch logic leaves content undefined when catalogItem?.spec.category is neither CatalogItemCategory.CatalogItemCategorySystem nor CatalogItemCategory.CatalogItemCategoryApplication, causing an empty wizard; update the conditional chain in InstallWizard.tsx (the content variable and the catalogItem?.spec.category checks) to include a final fallback branch that renders a user-facing message (for example an Alert or EmptyState) explaining the category is unsupported or unknown, so instead of leaving content undefined you return a clear fallback UI whenever the category is missing or unrecognized rather than rendering nothing.libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx-105-105 (1)
105-105:⚠️ Potential issue | 🟡 MinorTypo in prop name:
exisingLabelsshould beexistingLabels.The prop is consistently misspelled across the EditAppModal component definition and its usage. Update the prop name in EditAppModalProps, the component destructuring in EditAppModal, and the usage in ResourceCatalogPage.tsx.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx` at line 105, The prop name is misspelled as exisingLabels; rename it to existingLabels across the component interface and usages: update the EditAppModalProps type/interface to define existingLabels, change the destructured prop in the EditAppModal component to use existingLabels, and update every place that passes exisingLabels (including the call site in ResourceCatalogPage where exisingLabels={currentLabels} is used) to existingLabels={currentLabels}; also adjust any internal references, tests, and imports that reference the old name to avoid type errors.libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx-245-249 (1)
245-249:⚠️ Potential issue | 🟡 Minor
sort()mutates the source array from props.
catalogItem.spec.reference.artifacts?.sort(...)sorts the array in place, which mutates the prop data. This can cause subtle issues with React reconciliation or unexpected side effects if the samecatalogItemobject is referenced elsewhere.Proposed fix — spread before sorting
const artifacts = React.useMemo(() => { - return catalogItem.spec.reference.artifacts?.sort((a, b) => + return [...(catalogItem.spec.reference.artifacts || [])].sort((a, b) => getArtifactLabel(a, t).localeCompare(getArtifactLabel(b, t)), ); }, [catalogItem, t]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx` around lines 245 - 249, The current React.useMemo block creates artifacts by calling catalogItem.spec.reference.artifacts?.sort(...), which mutates the props array; change it to sort a shallow copy instead (e.g. clone with spread or slice before calling sort) so that artifacts is a new sorted array without mutating catalogItem.spec.reference.artifacts; update the React.useMemo that defines artifacts and keep the same comparator using getArtifactLabel(a, t).localeCompare(getArtifactLabel(b, t)).libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx-227-252 (1)
227-252:⚠️ Potential issue | 🟡 MinorStale closure:
onSelectionChangeis captured once and never updated.The
Visualizationcontroller is created in auseMemowith[]deps, but theSELECTION_EVENTlistener closes overonSelectionChangefrom the initial render. If the parent ever supplies a different callback, the old one is still invoked.A common fix is to store the callback in a ref:
Proposed fix
+ const onSelectionChangeRef = React.useRef(onSelectionChange); + onSelectionChangeRef.current = onSelectionChange; + const controller = React.useMemo(() => { const newController = new Visualization(); newController.registerComponentFactory(customComponentFactory); newController.registerLayoutFactory(customLayoutFactory); newController.addEventListener(GRAPH_LAYOUT_END_EVENT, () => { newController.getGraph().fit(80); }); newController.addEventListener(SELECTION_EVENT, (ids: string[]) => { const selectedId = ids[0]; if (selectedId) { const node = newController.getNodeById(selectedId); if (node) { const data = node.getData() as VersionNodeData | undefined; if (data?.entryName) { - onSelectionChange(selectedId, data.entryName); + onSelectionChangeRef.current(selectedId, data.entryName); } } } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsx` around lines 227 - 252, The SELECTION_EVENT listener inside the Visualization controller (created in the useMemo block that instantiates newController / Visualization) closes over the initial onSelectionChange and never sees updates; fix by storing onSelectionChange in a ref (e.g. selectionChangeRef) and updating that ref in a useEffect whenever onSelectionChange changes, then update the SELECTION_EVENT handler to call selectionChangeRef.current(selectedId, entryName) instead of the closed-over onSelectionChange; keep the controller creation in useMemo but reference the ref inside the listener so the latest callback is used.libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx-204-232 (1)
204-232:⚠️ Potential issue | 🟡 Minor
artifactTypeis typed asCatalogItemArtifactTypebut assigned translated strings.
let artifactType: CatalogItemArtifactType;declares the variable as an enum type, but every case branch assigns a human-readable translated string (e.g.,t('Bare Metal')). This is a type-safety gap that TypeScript may not catch if the enum values are plain strings. The variable should be typed asstring.- let artifactType: CatalogItemArtifactType; + let artifactType: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsx` around lines 204 - 232, The variable artifactType in SelectTargetStep.tsx is declared with the enum type CatalogItemArtifactType but is assigned translated human-readable strings in the switch on artifact.type; change the declaration to a string type (or let it be inferred) so artifactType holds the t(...) strings correctly and restore type-safety—update the line declaring artifactType and ensure any downstream uses accept a string rather than CatalogItemArtifactType.libs/ui-components/src/components/Catalog/InstallWizard/steps/ReviewStep.tsx-139-139 (1)
139-139:⚠️ Potential issue | 🟡 Minor
UpdateAlertsoutput is not wrapped in a<StackItem>.
<Stack hasGutter>expects<StackItem>children for proper spacing. The alerts rendered byUpdateAlertssit directly inside<Stack>, which may cause layout/gap issues.Proposed fix
- <UpdateAlerts catalogItem={catalogItem} /> + <StackItem> + <UpdateAlerts catalogItem={catalogItem} /> + </StackItem>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/steps/ReviewStep.tsx` at line 139, Wrap the UpdateAlerts output in a StackItem to satisfy the Stack's expected children and ensure consistent gutter spacing: in ReviewStep (where <Stack hasGutter> is used) replace the direct <UpdateAlerts catalogItem={catalogItem} /> child with a <StackItem> that contains <UpdateAlerts catalogItem={catalogItem} /> so UpdateAlerts is a proper StackItem child and layout/gap issues are resolved.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx-348-365 (1)
348-365:⚠️ Potential issue | 🟡 MinorEmpty object
{}pushed into actions array whenitemVersionis falsy.On Line 360, when there's no matching version, an empty
{}is added to the actions list. PatternFly'sActionsColumnmay render this as a blank/non-functional menu item. Use a separator or filter it out.💡 Proposed fix — filter out empty actions
- const actions: IAction[] = [ - itemVersion - ? { - title: t('Edit'), - onClick: () => - setAppToEdit({ - item: app.item, - name: app.name, - channel: appChannel, - version: itemVersion, - }), - } - : {}, - { - title: t('Delete'), - onClick: () => setAppToDelete(app.name), - }, - ]; + const actions: IAction[] = [ + ...(itemVersion + ? [ + { + title: t('Edit'), + onClick: () => + setAppToEdit({ + item: app.item, + name: app.name, + channel: appChannel, + version: itemVersion, + }), + }, + ] + : []), + { + title: t('Delete'), + onClick: () => setAppToDelete(app.name), + }, + ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 348 - 365, The actions array in InstalledSoftware.tsx currently pushes an empty object when itemVersion is falsy, producing a blank ActionsColumn entry; update the construction of actions (the variable named actions used for ActionsColumn) to only include the Edit action when itemVersion exists—either by conditionally pushing the Edit action (calling setAppToEdit) or by building the array and filtering out falsy/empty entries (e.g., filter(Boolean)) so no {} ends up in the array; ensure the Delete action (setAppToDelete) remains present and that references to setAppToEdit and setAppToDelete are unchanged.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx-249-269 (1)
249-269:⚠️ Potential issue | 🟡 MinorLoading state not reset when
appschanges.
appsLoadingstarts astruebut is only set tofalse— never back totruewhen theappsdependency changes. Iflabelsorspec.applicationsupdate after the first load, the UI will show staleappItemsuntil the new fetch completes.💡 Proposed fix
React.useEffect(() => { + setAppsLoading(true); (async () => { const appRequests = apps.map((app) => get<CatalogItem>(`catalogs/${app.catalog}/items/${app.item}`));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 249 - 269, In InstalledSoftware's useEffect (the async IIFE that fetches catalog items), ensure the loading state and displayed items are reset when the apps dependency changes by calling setAppsLoading(true) (and optionally setAppItems([])) before starting the Promise.allSettled fetch; this guarantees the UI shows a loading state and avoids stale appItems while get/CatalogItem requests complete.libs/ui-components/src/components/Catalog/UpdateModal/AppUpdateModal.tsx-185-196 (1)
185-196:⚠️ Potential issue | 🟡 MinorTypo:
exisingLabels→existingLabelsThe prop name is misspelled here and propagated through the component tree (also visible in
InstalledSoftware.tsx). Consider fixing it now to avoid a confusing public API surface.🔤 Proposed fix
type AppUpdateModalProps = { catalogItem: CatalogItem; onClose: VoidFunction; currentVersion: CatalogItemVersion; updates: CatalogItemVersion[]; onUpdate: (catalogItemVersion: CatalogItemVersion, channel: string, values: AppUpdateFormik) => Promise<void>; currentChannel: string; appSpec: ApplicationProviderSpec; - exisingLabels: Record<string, string> | undefined; + existingLabels: Record<string, string> | undefined; }; const AppUpdateModal: React.FC<AppUpdateModalProps> = ({ catalogItem, onClose, currentVersion, onUpdate, currentChannel, updates, appSpec, - exisingLabels, + existingLabels, }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/UpdateModal/AppUpdateModal.tsx` around lines 185 - 196, The prop name exisingLabels is misspelled; rename it to existingLabels across the component API and usages: update the AppUpdateModalProps type, the AppUpdateModal function parameter list (prop destructuring), and any references/props passed from callers such as InstalledSoftware.tsx to use existingLabels instead of exisingLabels, and update any downstream variable names/usages inside AppUpdateModal to match the corrected prop name.libs/ui-components/src/components/Catalog/utils.ts-179-180 (1)
179-180:⚠️ Potential issue | 🟡 MinorDrop fallback handling for
catalogItem.metadata.namein label values.These are API-backed resources, so
metadata.nameshould be used directly.♻️ Proposed fix
- [OS_ITEM_LABEL_KEY]: catalogItem.metadata.name || '', + [OS_ITEM_LABEL_KEY]: catalogItem.metadata.name, ... - [`${appSpec.name}.${APP_ITEM_LABEL_KEY}`]: catalogItem.metadata.name || '', + [`${appSpec.name}.${APP_ITEM_LABEL_KEY}`]: catalogItem.metadata.name,As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads".Also applies to: 281-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/utils.ts` around lines 179 - 180, The label generation is using fallbacks/optional chaining for metadata.name; update occurrences that set [OS_ITEM_LABEL_KEY] (and the second occurrence around the 281-282 area) to use catalogItem.metadata.name directly (remove the optional chaining and the || '' fallback) so the code references the API-backed resource name without defensive defaults.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
libs/ui-components/assets/flight-control-logo.pngis excluded by!**/*.pngpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (103)
apps/ocp-plugin/console-extensions.jsonapps/ocp-plugin/package.jsonapps/ocp-plugin/src/components/AppContext/AppContext.tsxapps/ocp-plugin/src/components/Catalog/CatalogInstallWizard.tsxapps/ocp-plugin/src/components/Catalog/CatalogPage.tsxapps/ocp-plugin/src/typings.d.tsapps/ocp-plugin/src/utils/apiCalls.tsapps/ocp-plugin/tsconfig.jsonapps/ocp-plugin/webpack.config.tsapps/standalone/package.jsonapps/standalone/src/app/routes.tsxapps/standalone/src/app/utils/apiCalls.tsapps/standalone/tsconfig.jsonapps/standalone/webpack.config.tslibs/i18n/locales/en/translation.jsonlibs/types/alpha/index.tslibs/types/alpha/models/ApiVersion.tslibs/types/alpha/models/Catalog.tslibs/types/alpha/models/CatalogItem.tslibs/types/alpha/models/CatalogItemArtifact.tslibs/types/alpha/models/CatalogItemArtifactType.tslibs/types/alpha/models/CatalogItemCategory.tslibs/types/alpha/models/CatalogItemConfigurable.tslibs/types/alpha/models/CatalogItemDeprecation.tslibs/types/alpha/models/CatalogItemList.tslibs/types/alpha/models/CatalogItemMeta.tslibs/types/alpha/models/CatalogItemReference.tslibs/types/alpha/models/CatalogItemSpec.tslibs/types/alpha/models/CatalogItemType.tslibs/types/alpha/models/CatalogItemVersion.tslibs/types/alpha/models/CatalogItemVisibility.tslibs/types/alpha/models/CatalogList.tslibs/types/alpha/models/CatalogSpec.tslibs/types/alpha/models/CatalogStatus.tslibs/types/alpha/models/Status.tslibs/types/package.jsonlibs/types/scripts/openapi-typescript.jslibs/types/scripts/openapi-utils.jslibs/ui-components/package.jsonlibs/ui-components/src/components/Catalog/CatalogItemCard.tsxlibs/ui-components/src/components/Catalog/CatalogItemDetails.csslibs/ui-components/src/components/Catalog/CatalogItemDetails.tsxlibs/ui-components/src/components/Catalog/CatalogPage.csslibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/CatalogPageToolbar.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallOsWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/InstallWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/AppConfigStep.csslibs/ui-components/src/components/Catalog/InstallWizard/steps/AppConfigStep.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/ReviewStep.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/SelectTargetStep.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsxlibs/ui-components/src/components/Catalog/InstallWizard/types.tslibs/ui-components/src/components/Catalog/InstallWizard/utils.tslibs/ui-components/src/components/Catalog/InstalledSoftware.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.csslibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Catalog/UpdateModal/AppUpdateModal.tsxlibs/ui-components/src/components/Catalog/UpdateModal/EditAppModal.tsxlibs/ui-components/src/components/Catalog/UpdateModal/EditOsModal.tsxlibs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsxlibs/ui-components/src/components/Catalog/UpdateModal/UpdateGraph.tsxlibs/ui-components/src/components/Catalog/const.tslibs/ui-components/src/components/Catalog/useCatalogFilter.tslibs/ui-components/src/components/Catalog/useCatalogs.tslibs/ui-components/src/components/Catalog/useSubmitCatalogForm.tslibs/ui-components/src/components/Catalog/utils.tslibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsCatalog.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsxlibs/ui-components/src/components/Device/DevicesPage/DevicesPage.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsxlibs/ui-components/src/components/Device/DevicesPage/useDevices.tslibs/ui-components/src/components/Device/EditDeviceWizard/EditDeviceWizard.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/ApplicationTemplates.tsxlibs/ui-components/src/components/Device/EditDeviceWizard/steps/DeviceTemplateStep.tsxlibs/ui-components/src/components/DynamicForm/DynamicForm.tsxlibs/ui-components/src/components/DynamicForm/FieldErrors.tsxlibs/ui-components/src/components/DynamicForm/FieldTemplate.tsxlibs/ui-components/src/components/DynamicForm/FormWidget.tsxlibs/ui-components/src/components/DynamicForm/VolumeImageField.tsxlibs/ui-components/src/components/Fleet/CreateFleet/CreateFleetWizard.tsxlibs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsCatalog.tsxlibs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsPage.tsxlibs/ui-components/src/components/Fleet/FleetRow.tsxlibs/ui-components/src/components/Fleet/FleetsPage.tsxlibs/ui-components/src/components/Fleet/useFleets.tslibs/ui-components/src/components/Table/Table.tsxlibs/ui-components/src/components/common/CodeEditor/YamlEditor.tsxlibs/ui-components/src/components/common/CodeEditor/YamlEditorBase.tsxlibs/ui-components/src/components/common/FlightCtlWizardFooter.tsxlibs/ui-components/src/components/common/LabelsView.tsxlibs/ui-components/src/components/common/PageNavigation.tsxlibs/ui-components/src/components/common/WithTooltip.tsxlibs/ui-components/src/components/form/FormSelect.tsxlibs/ui-components/src/components/form/LabelsField.tsxlibs/ui-components/src/constants.tslibs/ui-components/src/hooks/useAppContext.tsxlibs/ui-components/src/hooks/useNavigate.tsxlibs/ui-components/src/utils/api.tslibs/ui-components/tsconfig.json
| CatalogItemTypeDriver = 'driver', | ||
| CatalogItemTypeContainer = 'container', | ||
| CatalogItemTypeHelm = 'helm', | ||
| CatalogItemTypeQuadlet = 'quadlet', |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -rn "CatalogItemTypeQuadlet|quadlet" --type=tsx --type=ts -B 3 -A 3 libs/ui-components/src/ || echo "No matches found in libs/ui-components/src"Repository: flightctl/flightctl-ui
Length of output: 153
🏁 Script executed:
# Also check other UI-related directories
fd -e ts -e tsx libs/ -path "*/src/*" | head -20Repository: flightctl/flightctl-ui
Length of output: 234
🏁 Script executed:
# Check if there are any feature flags or guards related to quadlet
rg -rn "quadlet|Quadlet" --type=tsx --type=ts -B 2 -A 2 libs/ | head -50Repository: flightctl/flightctl-ui
Length of output: 93
🏁 Script executed:
# Search for any usage of quadlet/Quadlet in the entire repository
rg -i "quadlettype|catalogitemtype.*quadlet" --type ts -B 2 -A 2 || echo "No matches found"Repository: flightctl/flightctl-ui
Length of output: 2449
🏁 Script executed:
# List files in libs to understand structure
ls -la libs/Repository: flightctl/flightctl-ui
Length of output: 399
🏁 Script executed:
# Check if CatalogItemTypeQuadlet is referenced anywhere in the codebase
rg "CatalogItemTypeQuadlet" -B 3 -A 3Repository: flightctl/flightctl-ui
Length of output: 3222
🏁 Script executed:
# Check CatalogPage.tsx for any feature flags around quadlet
rg -B 10 -A 10 "CatalogItemType.CatalogItemTypeQuadlet" libs/ui-components/src/components/Catalog/CatalogPage.tsxRepository: flightctl/flightctl-ui
Length of output: 785
🏁 Script executed:
# Check useCatalogs.ts for any feature flags
rg -B 10 -A 10 "appTypeIds" libs/ui-components/src/components/Catalog/useCatalogs.tsRepository: flightctl/flightctl-ui
Length of output: 1597
🏁 Script executed:
# Search for any feature flags or environment variables that might gate quadlet
rg "feature|flag|FEATURE|FLAG|experimental|EXPERIMENTAL" libs/ui-components/src/components/Catalog/ | grep -i quadlet || echo "No feature flags found for quadlet"Repository: flightctl/flightctl-ui
Length of output: 100
Remove or guard CatalogItemTypeQuadlet from the UI until backend support is available.
The enum value itself is acceptable in the generated types file, but verification shows quadlet is fully exposed in the UI without any feature flags or guards:
CatalogPage.tsx: Quadlet appears in the catalog filter UI with full translationuseCatalogs.ts: Included inappTypeIdsarrayutils.ts: Mapped toAppType.AppTypeQuadlet
Per PR #371 learnings, quadlet support is preparatory only and full UI implementation should be deferred until backend support ships. Either disable the quadlet filter option or add a feature flag to prevent users from selecting unsupported catalog items.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/types/alpha/models/CatalogItemType.ts` at line 14,
CatalogItemTypeQuadlet is exposed in the UI but backend doesn't support it;
remove or guard its usage so users can't select it. Update CatalogPage.tsx to
hide/remove the quadlet option from the catalog filter UI (and its translation
key), update useCatalogs.ts to remove CatalogItemTypeQuadlet from the appTypeIds
array, and update utils.ts to stop mapping CatalogItemTypeQuadlet to
AppType.AppTypeQuadlet (or wrap all three places with a feature flag check e.g.,
isQuadletEnabled()). Ensure the same guard or flag is used consistently across
CatalogPage.tsx, useCatalogs.ts, and utils.ts so quadlet remains available only
when backend support is present.
| export const getDefaultChannelAndVersion = (item: CatalogItem) => { | ||
| if (!item.spec.versions.length) { | ||
| return { | ||
| version: '', | ||
| channel: '', | ||
| }; | ||
| } | ||
|
|
||
| const versions = item.spec.versions.sort((v1, v2) => semver.rcompare(v1.version, v2.version)); | ||
|
|
||
| // release then prerelease | ||
| const latestVersion = versions.find((v) => !semver.prerelease(v.version)) || versions[0]; | ||
|
|
||
| return { | ||
| version: latestVersion.version, | ||
| channel: latestVersion.channels[0], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Array.sort() mutates item.spec.versions in place.
item.spec.versions.sort(...) sorts the original array, which could cause unexpected side effects if other code relies on the original version order. Create a shallow copy first.
🐛 Proposed fix
export const getDefaultChannelAndVersion = (item: CatalogItem) => {
if (!item.spec.versions.length) {
return {
version: '',
channel: '',
};
}
- const versions = item.spec.versions.sort((v1, v2) => semver.rcompare(v1.version, v2.version));
+ const versions = [...item.spec.versions].sort((v1, v2) => semver.rcompare(v1.version, v2.version));
// release then prerelease
const latestVersion = versions.find((v) => !semver.prerelease(v.version)) || versions[0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getDefaultChannelAndVersion = (item: CatalogItem) => { | |
| if (!item.spec.versions.length) { | |
| return { | |
| version: '', | |
| channel: '', | |
| }; | |
| } | |
| const versions = item.spec.versions.sort((v1, v2) => semver.rcompare(v1.version, v2.version)); | |
| // release then prerelease | |
| const latestVersion = versions.find((v) => !semver.prerelease(v.version)) || versions[0]; | |
| return { | |
| version: latestVersion.version, | |
| channel: latestVersion.channels[0], | |
| }; | |
| }; | |
| export const getDefaultChannelAndVersion = (item: CatalogItem) => { | |
| if (!item.spec.versions.length) { | |
| return { | |
| version: '', | |
| channel: '', | |
| }; | |
| } | |
| const versions = [...item.spec.versions].sort((v1, v2) => semver.rcompare(v1.version, v2.version)); | |
| // release then prerelease | |
| const latestVersion = versions.find((v) => !semver.prerelease(v.version)) || versions[0]; | |
| return { | |
| version: latestVersion.version, | |
| channel: latestVersion.channels[0], | |
| }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/CatalogItemDetails.tsx` around
lines 257 - 274, The function getDefaultChannelAndVersion currently calls
item.spec.versions.sort(...), which mutates the original array; create a shallow
copy (e.g., via [...item.spec.versions] or item.spec.versions.slice()) before
sorting so you don't mutate item.spec.versions, then perform the semver.rcompare
sort and the existing prerelease selection logic on that copied array to return
version and channel.
| const { onSubmit, error, schemaErrors, setError } = useSubmitCatalogForm<InstallAppFormik>(async (values) => { | ||
| if (values.target !== 'fleet' && values.target !== 'device') { | ||
| return; | ||
| } | ||
| const selectedDevice = values.device; | ||
| const selectedFleet = values.fleet; | ||
| const installToDevice = values.target === 'device'; | ||
| const resourceId = installToDevice | ||
| ? `devices/${selectedDevice?.metadata.name}` | ||
| : `fleets/${selectedFleet?.metadata.name}`; | ||
|
|
||
| const res = await get<Device | Fleet>(resourceId); | ||
| const currentLabels = res?.metadata.labels; | ||
| const specPath = installToDevice ? '/' : '/spec/template/'; | ||
| const catalogItemVersion = catalogItem.spec.versions.find((v) => v.version === values.version); | ||
|
|
||
| if (!catalogItemVersion || !values.channel) { | ||
| return; | ||
| } | ||
| const currentApps = installToDevice | ||
| ? (res as Device)?.spec?.applications | ||
| : (res as Fleet)?.spec.template.spec.applications; | ||
| const allPatches = getAppPatches({ | ||
| appName: values.appName, | ||
| currentApps, | ||
| currentLabels, | ||
| catalogItem, | ||
| catalogItemVersion, | ||
| channel: values.channel, | ||
| formValues: | ||
| values.configureVia === 'editor' ? (load(values.editorContent) as Record<string, unknown>) : values.formValues, | ||
| specPath, | ||
| selectedAssets: values.configureVia === 'form' ? values.selectedAssets : [], | ||
| }); | ||
| if (!allPatches.length) { | ||
| return; | ||
| } | ||
| await patch(resourceId, allPatches); | ||
| }); |
There was a problem hiding this comment.
Silent returns on Lines 179–181 and 197–199 leave the user with no feedback.
When catalogItemVersion is not found or allPatches is empty, the submit handler returns silently — the wizard stays on the review step with no success or error indication. Consider either calling setError(...) or marking as successful (for the no-patches case, similar to how InstallOsWizard calls setIsSuccessful(true)).
💡 Proposed fix
if (!catalogItemVersion || !values.channel) {
+ throw t('Version {{version}} not found', { version: values.version });
- return;
}
...
if (!allPatches.length) {
- return;
+ // Nothing to change – treat as success
+ return; // onSubmit wrapper in useSubmitCatalogForm returns truthy → setIsSuccessful(true) in the caller
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsx`
around lines 163 - 201, Inside the onSubmit callback passed to
useSubmitCatalogForm in InstallAppWizard, avoid silent returns: when
catalogItemVersion is missing (the branch after checking if (!catalogItemVersion
|| !values.channel)) call setError(...) with a clear message (e.g., "Selected
version or channel not found") so the user sees feedback; and when allPatches is
empty (the if (!allPatches.length) branch) mark the wizard as successful instead
of returning silently—mirror the InstallOsWizard approach by invoking the
success flag (e.g., call setIsSuccessful(true) if available from the form/wizard
hook) or fallback to setError("No changes required for this install") so users
get explicit feedback. Ensure these changes are made inside the same anonymous
async function passed to useSubmitCatalogForm so the UI receives the appropriate
success/error state.
| return acc; | ||
| }, {}); | ||
|
|
||
| const versions = catalogItem.spec.versions.sort((v1, v2) => semver.compare(v2.version, v1.version)); |
There was a problem hiding this comment.
Array.sort() mutates catalogItem.spec.versions in place — same issue as CatalogItemDetails.tsx.
This can cause subtle bugs if any parent or sibling component relies on the original version order.
🐛 Proposed fix
- const versions = catalogItem.spec.versions.sort((v1, v2) => semver.compare(v2.version, v1.version));
+ const versions = [...catalogItem.spec.versions].sort((v1, v2) => semver.compare(v2.version, v1.version));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const versions = catalogItem.spec.versions.sort((v1, v2) => semver.compare(v2.version, v1.version)); | |
| const versions = [...catalogItem.spec.versions].sort((v1, v2) => semver.compare(v2.version, v1.version)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@libs/ui-components/src/components/Catalog/InstallWizard/steps/SpecificationsStep.tsx`
at line 63, catalogItem.spec.versions is being mutated by calling .sort()
directly; create a non-mutating copy before sorting (e.g., use spread or slice)
so the original array isn't changed, and assign the sorted result to the
existing versions variable used in SpecificationsStep.tsx (the line declaring
const versions and using semver.compare) to mirror the same fix applied in
CatalogItemDetails.tsx.
libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
Outdated
Show resolved
Hide resolved
| if (!volumes || volumes.length <= volumeIndex) { | ||
| return acc; | ||
| } | ||
| const volumeName = volumes[volumeIndex].name; | ||
|
|
There was a problem hiding this comment.
Guard negative volumeIndex before dereferencing volumes[volumeIndex].
The current check only handles upper bounds; a negative index slips through and can crash on .name.
♻️ Proposed fix
- if (!volumes || volumes.length <= volumeIndex) {
+ if (!volumes || volumeIndex < 0 || volumeIndex >= volumes.length) {
return acc;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!volumes || volumes.length <= volumeIndex) { | |
| return acc; | |
| } | |
| const volumeName = volumes[volumeIndex].name; | |
| if (!volumes || volumeIndex < 0 || volumeIndex >= volumes.length) { | |
| return acc; | |
| } | |
| const volumeName = volumes[volumeIndex].name; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/utils.ts` around lines 264 - 268,
The code dereferences volumes[volumeIndex] to get volumeName without guarding
against negative indexes; update the guard in the block that checks volumes and
volumeIndex so it returns acc when volumeIndex is negative or out of range
(e.g., if (!volumes || volumeIndex < 0 || volumes.length <= volumeIndex) return
acc), ensuring you validate volumeIndex before computing volumeName to avoid
crashes in utils.ts where volumeIndex, volumes, and volumeName are used.
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
Outdated
Show resolved
Hide resolved
| {!hideActions && ( | ||
| <Td isActionCell> | ||
| <ActionsColumn items={actions} /> | ||
| </Td> | ||
| )} |
There was a problem hiding this comment.
Keep table column structure consistent when actions are hidden.
Line 103 conditionally removes the action <Td>, but the table header still reserves an action column. That can create header/body column mismatch and visual/accessibility misalignment.
💡 Suggested fix
- {!hideActions && (
- <Td isActionCell>
- <ActionsColumn items={actions} />
- </Td>
- )}
+ {hideActions ? (
+ <Td />
+ ) : (
+ <Td isActionCell>
+ <ActionsColumn items={actions} />
+ </Td>
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {!hideActions && ( | |
| <Td isActionCell> | |
| <ActionsColumn items={actions} /> | |
| </Td> | |
| )} | |
| {hideActions ? ( | |
| <Td /> | |
| ) : ( | |
| <Td isActionCell> | |
| <ActionsColumn items={actions} /> | |
| </Td> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Fleet/FleetRow.tsx` around lines 103 - 107,
The conditional removal of the action <Td> in the FleetRow component causes
header/body column mismatch when hideActions is true; instead of omitting the
cell, always render the action column to preserve table structure—when
hideActions is true render an empty placeholder <Td isActionCell> (or a visually
hidden/aria-hidden placeholder) in the same spot as the <ActionsColumn
items={actions}> so header alignment and accessibility remain correct while
hiding the interactive controls.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
303-304: Drop the|| ''fallback onitem.metadata.name.
itemis aCatalogItemreturned from the API, sometadata.nameis always defined. The fallback contradicts the project guideline and was flagged in a prior review.Proposed fix
- assetItemName: item.metadata.name || '', + assetItemName: item.metadata.name,As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 303 - 304, Remove the unnecessary empty-string fallback on item.metadata.name inside the VolumeImageField component: replace the expression assetItemName: item.metadata.name || '' with assetItemName: item.metadata.name (leave assetCatalog: item.metadata.catalog unchanged) so metadata.name is treated as always defined per project guidelines.libs/ui-components/src/components/Catalog/useCatalogs.ts (1)
45-54: Operator precedence fix confirmed — LGTM.The previously flagged
categories?.length || 0 > 1andtypes.length || 0 > 1bugs have been corrected tocategories.length > 1andtypes.length > 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/useCatalogs.ts` around lines 45 - 54, In useCatalogs.ts the query assembly uses categories.length and types.length correctly but the types join still uses an unnecessary optional chain; update the spec.type multi-value branch to use types.join(',') (remove the `?.`) to match the categories branch and ensure consistency for parts.push calls (`parts.push(\`spec.type in (${types.join(',')})\`)`); confirm both branches for category and type produce the same CSV formatting and that parts is used downstream unchanged.
🧹 Nitpick comments (3)
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
219-220: Consider addingitemto the dependency array instead of suppressing the lint rule.The eslint-disable is safe here because
itemdoesn't change whileCatalogItemDetailsis mounted (it unmounts on "back"). However, includingitemin the deps would make the intent explicit and remove the need for the suppression comment, with no behavioral change.- // eslint-disable-next-line react-hooks/exhaustive-deps - const initialValues = React.useMemo(() => getDefaultChannelAndVersion(item), []); + const initialValues = React.useMemo(() => getDefaultChannelAndVersion(item), [item]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 219 - 220, Replace the eslint-disable hack by including item in the dependency array of the React.useMemo that computes initialValues: change the React.useMemo call that invokes getDefaultChannelAndVersion(item) so its deps array is [item] (and remove the "// eslint-disable-next-line react-hooks/exhaustive-deps" comment); this makes the dependency explicit while preserving behavior since item is stable while CatalogItemDetails is mounted.libs/ui-components/src/components/Catalog/useCatalogs.ts (1)
40-53:categories.length > 1branch is unreachable; optional chaining ontypesis redundant.
categoriesis only ever appended to at line 41 (one push site, one possible value:CatalogItemCategoryApplication), so its length is always 0 or 1 — theelse if (categories.length > 1)on line 47 is dead code. Separately,typesis always a defined array (initialized as[]on line 38), so the?.ontypes?.join(',')at line 53 is unnecessary.♻️ Proposed cleanup
- } else if (categories.length > 1) { - parts.push(`spec.category in (${categories.join(',')})`); - } + } if (types.length === 1) { parts.push(`spec.type==${types[0]}`); } else if (types.length > 1) { - parts.push(`spec.type in (${types?.join(',')})`); + parts.push(`spec.type in (${types.join(',')})`); }If the intent is to future-proof for additional category values, the branch should be kept but the
types?.joinstill cleaned up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/useCatalogs.ts` around lines 40 - 53, categories can never exceed length 1 because the only push is CatalogItemCategory.CatalogItemCategoryApplication, so remove the unreachable else-if branch checking categories.length > 1 (or keep it only if you intentionally plan to add more category push sites) and simplify the types join by removing the redundant optional chaining (replace types?.join(',') with types.join(',')); update the block around variables categories, types and parts (the spec.category/spec.type push logic) accordingly so you either keep a single categories.length === 1 branch or explicitly leave a placeholder comment for future multi-category support, and always use types.join(',') when constructing the spec.type in-clause.libs/ui-components/src/components/Catalog/InstallWizard/utils.ts (1)
26-31: Avoid duplicate version lookup in initial config derivation.Line 27 and Line 30 run the same
findcall; caching the matched version once improves clarity and avoids drift between schema/config selection logic.♻️ Suggested cleanup
- const configSchema = - catalogItem.spec.versions.find((v) => v.version === version)?.configSchema ?? - catalogItem?.spec.defaults?.configSchema; - let defaultConfig = - catalogItem.spec.versions.find((v) => v.version === version)?.config ?? catalogItem?.spec.defaults?.config; + const selectedVersion = catalogItem.spec.versions.find((v) => v.version === version); + const configSchema = selectedVersion?.configSchema ?? catalogItem.spec.defaults?.configSchema; + let defaultConfig = selectedVersion?.config ?? catalogItem.spec.defaults?.config;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/utils.ts` around lines 26 - 31, The duplicated find call should be replaced by caching the matched version first: compute a local const (e.g., matchedVersion) by calling catalogItem.spec.versions.find(v => v.version === version), then derive configSchema from matchedVersion?.configSchema ?? catalogItem?.spec.defaults?.configSchema and defaultConfig from matchedVersion?.config ?? catalogItem?.spec.defaults?.config; update the variables configSchema and defaultConfig in utils.ts to use this cached matchedVersion to avoid repeating the lookup and ensure consistent selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/i18n/locales/en/translation.json`:
- Around line 39-41: Revert the manual changes to translation.json and instead
add or fix the corresponding source-level i18n keys by using the t(...) calls
where the UI strings live (e.g., add t("Software Catalog"), t("Catalog"),
t("Install Catalog item") in the appropriate components), then run the
extraction/regeneration script (npm run i18n) to regenerate translation.json and
commit the regenerated file(s).
In
`@libs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsx`:
- Around line 170-172: The resourceId construction uses optional chaining
causing possible "devices/undefined" or "fleets/undefined"; modify the logic
around installToDevice to explicitly guard that selectedDevice or selectedFleet
exists (e.g., throw or return early if missing) and then access metadata.name
directly (replace selectedDevice?.metadata.name / selectedFleet?.metadata.name
with selectedDevice.metadata.name / selectedFleet.metadata.name after the
explicit existence check) so resourceId is always built from a defined resource
name.
---
Duplicate comments:
In `@libs/ui-components/src/components/Catalog/useCatalogs.ts`:
- Around line 45-54: In useCatalogs.ts the query assembly uses categories.length
and types.length correctly but the types join still uses an unnecessary optional
chain; update the spec.type multi-value branch to use types.join(',') (remove
the `?.`) to match the categories branch and ensure consistency for parts.push
calls (`parts.push(\`spec.type in (${types.join(',')})\`)`); confirm both
branches for category and type produce the same CSV formatting and that parts is
used downstream unchanged.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 303-304: Remove the unnecessary empty-string fallback on
item.metadata.name inside the VolumeImageField component: replace the expression
assetItemName: item.metadata.name || '' with assetItemName: item.metadata.name
(leave assetCatalog: item.metadata.catalog unchanged) so metadata.name is
treated as always defined per project guidelines.
---
Nitpick comments:
In `@libs/ui-components/src/components/Catalog/InstallWizard/utils.ts`:
- Around line 26-31: The duplicated find call should be replaced by caching the
matched version first: compute a local const (e.g., matchedVersion) by calling
catalogItem.spec.versions.find(v => v.version === version), then derive
configSchema from matchedVersion?.configSchema ??
catalogItem?.spec.defaults?.configSchema and defaultConfig from
matchedVersion?.config ?? catalogItem?.spec.defaults?.config; update the
variables configSchema and defaultConfig in utils.ts to use this cached
matchedVersion to avoid repeating the lookup and ensure consistent selection.
In `@libs/ui-components/src/components/Catalog/useCatalogs.ts`:
- Around line 40-53: categories can never exceed length 1 because the only push
is CatalogItemCategory.CatalogItemCategoryApplication, so remove the unreachable
else-if branch checking categories.length > 1 (or keep it only if you
intentionally plan to add more category push sites) and simplify the types join
by removing the redundant optional chaining (replace types?.join(',') with
types.join(',')); update the block around variables categories, types and parts
(the spec.category/spec.type push logic) accordingly so you either keep a single
categories.length === 1 branch or explicitly leave a placeholder comment for
future multi-category support, and always use types.join(',') when constructing
the spec.type in-clause.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 219-220: Replace the eslint-disable hack by including item in the
dependency array of the React.useMemo that computes initialValues: change the
React.useMemo call that invokes getDefaultChannelAndVersion(item) so its deps
array is [item] (and remove the "// eslint-disable-next-line
react-hooks/exhaustive-deps" comment); this makes the dependency explicit while
preserving behavior since item is stable while CatalogItemDetails is mounted.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsxlibs/ui-components/src/components/Catalog/InstallWizard/utils.tslibs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsxlibs/ui-components/src/components/Catalog/const.tslibs/ui-components/src/components/Catalog/useCatalogs.tslibs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-components/src/components/Catalog/UpdateModal/OsUpdateModal.tsx
| "Software Catalog": "Software Catalog", | ||
| "Catalog": "Catalog", | ||
| "Install Catalog item": "Install Catalog item", |
There was a problem hiding this comment.
Do not manually edit generated libs/i18n/locales/en/translation.json.
These string additions/changes should come from source code (t(...) usage) and then be regenerated via npm run i18n. Direct edits here can drift from extracted keys and break translation maintenance flow.
Recommended fix path
- Revert direct edits in
libs/i18n/locales/en/translation.json. - Add/fix the corresponding source strings in UI code.
- Run:
npm run i18n- Commit the regenerated locale file(s).
As per coding guidelines: "Do not manually edit translation.json in libs/i18n/locales/en/; regenerate with npm run i18n by adding or fixing keys in source code".
Also applies to: 190-326, 548-548, 639-639, 659-667, 1361-1361
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/i18n/locales/en/translation.json` around lines 39 - 41, Revert the
manual changes to translation.json and instead add or fix the corresponding
source-level i18n keys by using the t(...) calls where the UI strings live
(e.g., add t("Software Catalog"), t("Catalog"), t("Install Catalog item") in the
appropriate components), then run the extraction/regeneration script (npm run
i18n) to regenerate translation.json and commit the regenerated file(s).
| const resourceId = installToDevice | ||
| ? `devices/${selectedDevice?.metadata.name}` | ||
| : `fleets/${selectedFleet?.metadata.name}`; |
There was a problem hiding this comment.
Remove optional chaining from API resource name when building resourceId.
Line 171 and Line 172 can build invalid endpoints (devices/undefined / fleets/undefined). Guard selection explicitly, then access metadata.name directly.
✅ Suggested fix
- const resourceId = installToDevice
- ? `devices/${selectedDevice?.metadata.name}`
- : `fleets/${selectedFleet?.metadata.name}`;
+ let resourceId: string;
+ if (installToDevice) {
+ if (!selectedDevice) {
+ throw t('Device must be selected');
+ }
+ resourceId = `devices/${selectedDevice.metadata.name}`;
+ } else {
+ if (!selectedFleet) {
+ throw t('Fleet must be selected');
+ }
+ resourceId = `fleets/${selectedFleet.metadata.name}`;
+ }As per coding guidelines: Treat resource.metadata.name as always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const resourceId = installToDevice | |
| ? `devices/${selectedDevice?.metadata.name}` | |
| : `fleets/${selectedFleet?.metadata.name}`; | |
| let resourceId: string; | |
| if (installToDevice) { | |
| if (!selectedDevice) { | |
| throw t('Device must be selected'); | |
| } | |
| resourceId = `devices/${selectedDevice.metadata.name}`; | |
| } else { | |
| if (!selectedFleet) { | |
| throw t('Fleet must be selected'); | |
| } | |
| resourceId = `fleets/${selectedFleet.metadata.name}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/InstallWizard/InstallAppWizard.tsx`
around lines 170 - 172, The resourceId construction uses optional chaining
causing possible "devices/undefined" or "fleets/undefined"; modify the logic
around installToDevice to explicitly guard that selectedDevice or selectedFleet
exists (e.g., throw or return early if missing) and then access metadata.name
directly (replace selectedDevice?.metadata.name / selectedFleet?.metadata.name
with selectedDevice.metadata.name / selectedFleet.metadata.name after the
explicit existence check) so resourceId is always built from a defined resource
name.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
297-301: Drop the|| ''fallback onitem.metadata.name.
itemis aCatalogItemreturned from the API, sometadata.nameis always defined per project convention.♻️ Proposed fix
- assetItemName: item.metadata.name || '', + assetItemName: item.metadata.name,As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 297 - 301, The code is using a redundant fallback (|| '') for item.metadata.name even though CatalogItem.metadata.name is always defined; update the VolumeImageField logic that builds the reference (where assetItemName is set and onChange(reference) is called) to assign assetItemName: item.metadata.name without the empty-string fallback, ensuring any related typing remains satisfied and preserving the existing onChange(reference) call.libs/i18n/locales/en/translation.json (1)
39-41: Do not manually edittranslation.json— regenerate withnpm run i18n.These keys should be added via
t(...)calls in source code and then extracted automatically. Direct edits can drift from the actual source strings.As per coding guidelines: "Do not manually edit translation.json in libs/i18n/locales/en/; regenerate with
npm run i18nby adding or fixing keys in source code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/i18n/locales/en/translation.json` around lines 39 - 41, The translation.json file was edited directly to add keys ("Software Catalog", "Catalog", "Install Catalog item"); undo that manual change and instead add or correct the user-facing strings in the source code where they belong (use t("Software Catalog"), t("Catalog"), t("Install Catalog item") in the relevant React components / modules), then run the extraction step npm run i18n to regenerate libs/i18n/locales/en/translation.json so the keys are added automatically; ensure you remove the manual entries from translation.json before committing.
🧹 Nitpick comments (3)
libs/ui-components/src/components/common/WithHelperText.tsx (1)
11-13: Makecontentoptional inFormGroupWithHelperTextPropsto match the new conditional rendering.The runtime now skips the popover when
contentis falsy (line 55), but the type still requirescontent(inherited fromHelperTextPopoverProps). This means callers passingundefinedascontentare relying on runtime behavior that the TypeScript contract doesn't express, and a strict type-checker may warn depending on project settings.♻️ Proposed fix
-type FormGroupWithHelperTextProps = HelperTextPopoverProps & { +type FormGroupWithHelperTextProps = Omit<HelperTextPopoverProps, 'content'> & { + content?: React.ReactNode; isRequired?: boolean; };Also applies to: 47-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/WithHelperText.tsx` around lines 11 - 13, The prop type FormGroupWithHelperTextProps currently inherits content as required from HelperTextPopoverProps but the component conditionally skips rendering the popover when content is falsy; update the type so content is optional to match runtime behavior. Modify FormGroupWithHelperTextProps by overriding/omitting the inherited content (e.g., Omit<HelperTextPopoverProps, 'content'> and then add content?: HelperTextPopoverProps['content']) and keep isRequired?: boolean; ensure the updated type is used where WithHelperText or FormGroupWithHelperTextProps is referenced so TypeScript no longer requires content.libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx (1)
160-176: Stale expanded-state entries are never pruned on item removal.The
useEffectonly adds missing indices but never removes entries for deleted items. Over time (add → remove → add cycles),expandedItemsaccumulates orphan keys. While this won't break rendering, it's a minor memory/state hygiene concern, and could lead to unexpected behavior if the user removes item 1, then adds a new item that gets assigned the same index.Also, the
itemsarray is referenced inside the effect but onlyitems.lengthis in the dependency list. This works for the intended purpose but will trigger thereact-hooks/exhaustive-depslint rule.♻️ Suggested fix — rebuild state from current items
React.useEffect(() => { setExpandedItems((prev) => { - const next = { ...prev }; - items.forEach((item) => { - if (next[item.index] === undefined) next[item.index] = true; - }); + const next: Record<number, boolean> = {}; + items.forEach((item) => { + next[item.index] = prev[item.index] ?? true; + }); return next; }); - }, [items.length]); + }, [items]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx` around lines 160 - 176, The effect and initial state for expandedItems currently only add missing indices and never remove stale keys; update the logic in the useEffect (and adjust the dependency to [items]) to rebuild expandedItems from the current items list: create a new record from items (similar to the initial items.reduce) that preserves existing values from prev when present and defaults to true for new indices, and replace state via setExpandedItems(next) so removed item indices are pruned; reference the expandedItems/setExpandedItems state and the useEffect that currently depends on items.length and items.reduce for locating the change.libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
139-190: Consider moving the error/loading check before building the default content.Currently, the toolbar + gallery JSX (lines 139–176) is always constructed and then discarded when
errororisLoadingis truthy (lines 178–190). Moving the error/loading guard first avoids the unnecessary allocation.♻️ Suggested reorder
const AssetsList = ({ ... }: AssetsListProps) => { const { t } = useTranslation(); const hasFilters = !!nameFilter?.trim(); - let modalContent = ( - <> - <Toolbar ...> - ... - </Toolbar> - {/* gallery/empty state */} - </> - ); - - if (error) { - modalContent = ( ... ); - } else if (isLoading) { - modalContent = ( ... ); - } + let modalContent: React.ReactNode; + + if (error) { + modalContent = ( ... ); + } else if (isLoading) { + modalContent = ( ... ); + } else { + modalContent = ( + <> + <Toolbar ...> + ... + </Toolbar> + {/* gallery/empty state */} + </> + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 139 - 190, Move the error/isLoading guard before creating the default modalContent so we don't build the toolbar/gallery JSX only to discard it; in VolumeImageField.tsx check the error and isLoading flags first (the same conditions that set modalContent to the Alert or Spinner) and return or set modalContent to those states before constructing the default block that references assetCatalogItems, nameFilter, pagination, CatalogItemCard and onSelect, ensuring the Alert/Spinner branches run early and the heavy JSX is only allocated when neither error nor isLoading is true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@libs/i18n/locales/en/translation.json`:
- Around line 39-41: The translation.json file was edited directly to add keys
("Software Catalog", "Catalog", "Install Catalog item"); undo that manual change
and instead add or correct the user-facing strings in the source code where they
belong (use t("Software Catalog"), t("Catalog"), t("Install Catalog item") in
the relevant React components / modules), then run the extraction step npm run
i18n to regenerate libs/i18n/locales/en/translation.json so the keys are added
automatically; ensure you remove the manual entries from translation.json before
committing.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 297-301: The code is using a redundant fallback (|| '') for
item.metadata.name even though CatalogItem.metadata.name is always defined;
update the VolumeImageField logic that builds the reference (where assetItemName
is set and onChange(reference) is called) to assign assetItemName:
item.metadata.name without the empty-string fallback, ensuring any related
typing remains satisfied and preserving the existing onChange(reference) call.
---
Nitpick comments:
In `@libs/ui-components/src/components/common/WithHelperText.tsx`:
- Around line 11-13: The prop type FormGroupWithHelperTextProps currently
inherits content as required from HelperTextPopoverProps but the component
conditionally skips rendering the popover when content is falsy; update the type
so content is optional to match runtime behavior. Modify
FormGroupWithHelperTextProps by overriding/omitting the inherited content (e.g.,
Omit<HelperTextPopoverProps, 'content'> and then add content?:
HelperTextPopoverProps['content']) and keep isRequired?: boolean; ensure the
updated type is used where WithHelperText or FormGroupWithHelperTextProps is
referenced so TypeScript no longer requires content.
In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx`:
- Around line 160-176: The effect and initial state for expandedItems currently
only add missing indices and never remove stale keys; update the logic in the
useEffect (and adjust the dependency to [items]) to rebuild expandedItems from
the current items list: create a new record from items (similar to the initial
items.reduce) that preserves existing values from prev when present and defaults
to true for new indices, and replace state via setExpandedItems(next) so removed
item indices are pruned; reference the expandedItems/setExpandedItems state and
the useEffect that currently depends on items.length and items.reduce for
locating the change.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 139-190: Move the error/isLoading guard before creating the
default modalContent so we don't build the toolbar/gallery JSX only to discard
it; in VolumeImageField.tsx check the error and isLoading flags first (the same
conditions that set modalContent to the Alert or Spinner) and return or set
modalContent to those states before constructing the default block that
references assetCatalogItems, nameFilter, pagination, CatalogItemCard and
onSelect, ensuring the Alert/Spinner branches run early and the heavy JSX is
only allocated when neither error nor isLoading is true.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/DynamicForm/FieldTemplate.tsxlibs/ui-components/src/components/DynamicForm/VolumeImageField.tsxlibs/ui-components/src/components/common/WithHelperText.tsx
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/types/scripts/openapi-typescript.js (2)
109-114:⚠️ Potential issue | 🟡 MinorAlpha directories missing from the initial cleanup.
main()cleansmodels,imagebuilder,tmp-types, andtmp-imagebuilder-types, but never cleansalphaortmp-alpha-types. If a previous generation run placed a staleindex.tsinalpha/andgenerateTypes(ALPHA_CORE_API)fails before reaching line 97, that stale file persists silently.rimraf(finalDir)insidegenerateTypesonly coversalpha/models, not thealpha/root.🛠️ Proposed fix
await Promise.all([ rimraf(path.join(rootDir, 'models')), rimraf(path.join(rootDir, 'imagebuilder')), + rimraf(path.join(rootDir, 'alpha')), rimraf(path.join(rootDir, 'tmp-types')), rimraf(path.join(rootDir, 'tmp-imagebuilder-types')), + rimraf(path.join(rootDir, 'tmp-alpha-types')), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 109 - 114, main() currently omits cleaning the alpha root and its temp types, so stale files in alpha/ can persist; update the initial Promise.all cleanup to also rimraf(path.join(rootDir, 'alpha')) and rimraf(path.join(rootDir, 'tmp-alpha-types')) so both the alpha root and its temporary types are removed before calling generateTypes(ALPHA_CORE_API); keep the existing rimraf(finalDir) inside generateTypes() as-is but ensure the top-level cleanup includes these two additional rimraf calls to prevent leftover index.ts files.
55-57:⚠️ Potential issue | 🟡 MinorStale error message omits
alphacore.- throw new Error(`Unknown mode: ${mode}. Use 'core' or 'imagebuilder'`); + throw new Error(`Unknown mode: ${mode}. Use 'core', 'alphacore', or 'imagebuilder'`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` around lines 55 - 57, The error thrown when config[mode] is falsy uses a stale message listing only 'core' and 'imagebuilder'; update the throw in the block that checks config[mode] to include the actual valid modes (either add 'alphacore' to the message or dynamically list Object.keys(config) so the message stays accurate). Locate the check using the variables config and mode and replace the literal message with one that shows the available modes (e.g., `Unknown mode: ${mode}. Use ${Object.keys(config).join(', ')}`).
♻️ Duplicate comments (2)
libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx (1)
297-299:⚠️ Potential issue | 🟡 MinorRemove the fallback on API-backed
metadata.name.Line 298 still uses
item.metadata.name || ''. For API-returnedCatalogItem,metadata.nameshould be treated as always defined.♻️ Proposed fix
- assetItemName: item.metadata.name || '', + assetItemName: item.metadata.name,As per coding guidelines, "Treat
resource.metadata.nameas always defined for resources returned from the API or created resources, and avoid optional chaining in these contexts; only handle undefined for partial or input payloads".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx` around lines 297 - 299, The assignment that sets assetItemName currently uses a fallback (item.metadata.name || ''), but for API-backed CatalogItem metadata.name is always defined; update the code that constructs the object (the assetItemName property in the block where assetCatalog and assetItemName are set) to remove the fallback and use item.metadata.name directly so it no longer supplies an empty-string default.libs/types/scripts/openapi-typescript.js (1)
91-91:⚠️ Potential issue | 🟡 MinorStale comment still references "imagebuilder/index.ts".
This branch now handles both imagebuilder and alpha; the comment should be updated accordingly.
- // Copy the generated index.ts to imagebuilder/index.ts + // Copy the generated index.ts to the mode-specific output directory🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-typescript.js` at line 91, Update the stale inline comment that currently reads "// Copy the generated index.ts to imagebuilder/index.ts" to reference both targets handled by this branch (imagebuilder and alpha); locate that comment string in libs/types/scripts/openapi-typescript.js and change it to something like "// Copy the generated index.ts to imagebuilder/index.ts and alpha/index.ts" so it accurately reflects both destinations.
🧹 Nitpick comments (2)
libs/types/scripts/openapi-utils.js (1)
72-75: The comment about escaping special characters is misleading since no escaping is actually performed.The prefixes currently used ('v1beta1_openapi_yaml_components_schemas' and 'core_v1beta1_openapi_yaml_components_schemas') are safe, but if a future caller passes a prefix containing regex special characters (e.g.,
.,+), the constructed patterns would silently misbehave. Consider either removing the misleading comment or adding actual escaping for defensive robustness.♻️ Optional: escape the prefix for regex safety
- // Pre-construct the Regex patterns using the dynamic prefix - // We escape special characters in the prefix if needed, though usually not for schema names - const importRegex = new RegExp(`from\\s+['"]\\.\\/${prefix}_([A-Za-z][A-Za-z0-9]*)['"]`, 'g'); - const typeRegex = new RegExp(`\\b${prefix}_([A-Za-z][A-Za-z0-9]*)\\b`, 'g'); + // Pre-construct the Regex patterns using the dynamic prefix + const escapedPrefix = prefix.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const importRegex = new RegExp(`from\\s+['"]\\.\\/${escapedPrefix}_([A-Za-z][A-Za-z0-9]*)['"]`, 'g'); + const typeRegex = new RegExp(`\\b${escapedPrefix}_([A-Za-z][A-Za-z0-9]*)\\b`, 'g');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/types/scripts/openapi-utils.js` around lines 72 - 75, The comment claims special-character escaping is done for the dynamic prefix but no escaping occurs; update the code that builds importRegex and typeRegex to first escape regex metacharacters in the prefix (create or reuse an escapeRegExp helper that replaces [.*+?^${}()|[\]\\] with escaped versions) and then use the escapedPrefix when constructing new RegExp for importRegex and typeRegex (referencing the constants importRegex and typeRegex), or alternatively remove/clarify the misleading comment if you prefer not to add escaping.libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx (1)
126-128:CustomObjectFieldis a no-op passthrough — consider removing it.The current implementation unconditionally delegates to
DefaultObjectFieldwith no customization. Registering a passthrough adds indirection without value and makes future readers assume there's special logic.Either inline
DefaultObjectFielddirectly inpfFields, or keep the wrapper only when actual custom behavior is added.♻️ Proposed simplification
-// Custom Object Field - can be extended for other special object types -const CustomObjectField: React.FC<FieldProps> = (props) => { - return <DefaultObjectField {...props} />; -}; - // Custom fields registry const pfFields: RegistryFieldsType = { - ObjectField: CustomObjectField, + ObjectField: DefaultObjectField, StringField: CustomStringField, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx` around lines 126 - 128, CustomObjectField is an unconditional passthrough to DefaultObjectField; remove the unnecessary wrapper to reduce indirection by replacing references to CustomObjectField in pfFields with DefaultObjectField (or delete CustomObjectField and inline DefaultObjectField where pfFields is constructed). If you prefer to keep a named export for future customization, add a comment explaining it's intentionally a passthrough. Update imports/exports accordingly to avoid unused symbol errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/types/scripts/openapi-typescript.js`:
- Around line 39-45: ALPHA_CORE_API's coreRef value is inconsistent with
IMAGEBUILDER_API which can cause fixCoreReferences to miss prefixed type refs;
update the ALPHA_CORE_API configuration's coreRef to match IMAGEBUILDER_API's
'core_v1beta1_openapi_yaml_components_schemas' (or make both the same canonical
string) so the fixCoreReferences step and any future openapi-typescript-codegen
outputs will be transformed consistently; locate the ALPHA_CORE_API object and
change its coreRef field accordingly and verify fixCoreReferences uses that same
key when normalizing imports.
In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx`:
- Around line 160-177: The expandedItems state currently uses item.index which
becomes stale when array items are removed/shifted; change the state map to use
rjsf's stable item.key instead and update all places that read/write
expandedItems (including the toggle handler and any expand checks) to use
item.key; initialize expandedItems by reducing items.map(i => i.key) to true and
modify the React.useEffect to rebuild/merge state by iterating current items and
producing a new object that keeps existing values for existing item.key entries
and drops any keys that no longer exist (so removed items are cleaned up), then
call setExpandedItems with that cleaned map.
---
Outside diff comments:
In `@libs/types/scripts/openapi-typescript.js`:
- Around line 109-114: main() currently omits cleaning the alpha root and its
temp types, so stale files in alpha/ can persist; update the initial Promise.all
cleanup to also rimraf(path.join(rootDir, 'alpha')) and
rimraf(path.join(rootDir, 'tmp-alpha-types')) so both the alpha root and its
temporary types are removed before calling generateTypes(ALPHA_CORE_API); keep
the existing rimraf(finalDir) inside generateTypes() as-is but ensure the
top-level cleanup includes these two additional rimraf calls to prevent leftover
index.ts files.
- Around line 55-57: The error thrown when config[mode] is falsy uses a stale
message listing only 'core' and 'imagebuilder'; update the throw in the block
that checks config[mode] to include the actual valid modes (either add
'alphacore' to the message or dynamically list Object.keys(config) so the
message stays accurate). Locate the check using the variables config and mode
and replace the literal message with one that shows the available modes (e.g.,
`Unknown mode: ${mode}. Use ${Object.keys(config).join(', ')}`).
---
Duplicate comments:
In `@libs/types/scripts/openapi-typescript.js`:
- Line 91: Update the stale inline comment that currently reads "// Copy the
generated index.ts to imagebuilder/index.ts" to reference both targets handled
by this branch (imagebuilder and alpha); locate that comment string in
libs/types/scripts/openapi-typescript.js and change it to something like "//
Copy the generated index.ts to imagebuilder/index.ts and alpha/index.ts" so it
accurately reflects both destinations.
In `@libs/ui-components/src/components/DynamicForm/VolumeImageField.tsx`:
- Around line 297-299: The assignment that sets assetItemName currently uses a
fallback (item.metadata.name || ''), but for API-backed CatalogItem
metadata.name is always defined; update the code that constructs the object (the
assetItemName property in the block where assetCatalog and assetItemName are
set) to remove the fallback and use item.metadata.name directly so it no longer
supplies an empty-string default.
---
Nitpick comments:
In `@libs/types/scripts/openapi-utils.js`:
- Around line 72-75: The comment claims special-character escaping is done for
the dynamic prefix but no escaping occurs; update the code that builds
importRegex and typeRegex to first escape regex metacharacters in the prefix
(create or reuse an escapeRegExp helper that replaces [.*+?^${}()|[\]\\] with
escaped versions) and then use the escapedPrefix when constructing new RegExp
for importRegex and typeRegex (referencing the constants importRegex and
typeRegex), or alternatively remove/clarify the misleading comment if you prefer
not to add escaping.
In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx`:
- Around line 126-128: CustomObjectField is an unconditional passthrough to
DefaultObjectField; remove the unnecessary wrapper to reduce indirection by
replacing references to CustomObjectField in pfFields with DefaultObjectField
(or delete CustomObjectField and inline DefaultObjectField where pfFields is
constructed). If you prefer to keep a named export for future customization, add
a comment explaining it's intentionally a passthrough. Update imports/exports
accordingly to avoid unused symbol errors.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
libs/types/scripts/openapi-typescript.jslibs/types/scripts/openapi-utils.jslibs/ui-components/src/components/DynamicForm/FieldTemplate.tsxlibs/ui-components/src/components/DynamicForm/VolumeImageField.tsx
| [ALPHA_CORE_API]: { | ||
| swaggerUrl: getSwaggerUrl('core/v1alpha1'), | ||
| output: path.resolve(__dirname, '../tmp-alpha-types'), | ||
| finalDir: path.resolve(__dirname, '../alpha/models'), | ||
| coreRef: 'v1beta1_openapi_yaml_components_schemas', | ||
| outputDir: path.resolve(__dirname, '../alpha'), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check fixCoreReferences implementation and coreRef consistency
# 1. Show the fixCoreReferences implementation
echo "=== fixCoreReferences Implementation ==="
rg -n "fixCoreReferences" --type js -A 25
# 2. Check the full configuration block for both ALPHA_CORE_API and IMAGEBUILDER_API
echo -e "\n=== Checking coreRef values in config ==="
rg -n "coreRef:" libs/types/scripts/openapi-typescript.js -B 3
# 3. Check if any alpha model files exist
echo -e "\n=== Checking alpha models directory ==="
find libs/types/alpha/models -type f -name "*.ts" 2>/dev/null | head -5Repository: flightctl/flightctl-ui
Length of output: 6905
🏁 Script executed:
#!/bin/bash
# Check what reference patterns are actually in the generated alpha model files
echo "=== Sample of alpha model file content (first file) ==="
head -50 libs/types/alpha/models/ApiVersion.ts
echo -e "\n=== Search for openapi_yaml_components_schemas patterns in alpha models ==="
rg "openapi_yaml_components_schemas" libs/types/alpha/models --type ts -A 1 -B 1 | head -50
echo -e "\n=== Search for openapi_yaml_components_schemas patterns in imagebuilder models ==="
rg "openapi_yaml_components_schemas" libs/types/imagebuilder/models --type ts -A 1 -B 1 | head -50
echo -e "\n=== Count of references in each directory ==="
echo "Alpha references:"
rg "openapi_yaml_components_schemas" libs/types/alpha/models --type ts | wc -l
echo "Imagebuilder references:"
rg "openapi_yaml_components_schemas" libs/types/imagebuilder/models --type ts | wc -lRepository: flightctl/flightctl-ui
Length of output: 913
🏁 Script executed:
#!/bin/bash
# Check what type references and imports actually exist in the generated files
echo "=== Alpha model file imports and references ==="
rg "^import|^export|from.*models" libs/types/alpha/models --type ts | head -30
echo -e "\n=== Imagebuilder model file imports and references ==="
rg "^import|^export|from.*models" libs/types/imagebuilder/models --type ts | head -30
echo -e "\n=== Check if there are any inter-model dependencies in alpha ==="
rg "from.*\./" libs/types/alpha/models --type ts | head -20
echo -e "\n=== Check if there are any inter-model dependencies in imagebuilder ==="
rg "from.*\./" libs/types/imagebuilder/models --type ts | head -20
echo -e "\n=== Git log to see if models were recently regenerated ==="
git log --oneline -n 5 -- libs/types/alpha/models/ libs/types/imagebuilder/models/ 2>/dev/null || echo "No recent commits"Repository: flightctl/flightctl-ui
Length of output: 10552
🏁 Script executed:
#!/bin/bash
# Check the recent commit to understand what changes were made to the generation script
echo "=== Recent commit that changed the generation script ==="
git show --stat 7854c04
echo -e "\n=== Diff of the generation script changes ==="
git show 7854c04 -- libs/types/scripts/openapi-typescript.js | head -100Repository: flightctl/flightctl-ui
Length of output: 50378
Ensure coreRef values are consistent between configurations for future-proofing.
While the current generated alpha and imagebuilder models both contain clean, properly-formed imports without prefixed references, the coreRef values differ: ALPHA_CORE_API uses 'v1beta1_openapi_yaml_components_schemas' while IMAGEBUILDER_API uses 'core_v1beta1_openapi_yaml_components_schemas'. If openapi-typescript-codegen ever emits prefixed type references for the alpha spec matching the imagebuilder pattern, the fixCoreReferences step would silently fail to transform them. Align these coreRef values to ensure consistent handling across both API configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/types/scripts/openapi-typescript.js` around lines 39 - 45,
ALPHA_CORE_API's coreRef value is inconsistent with IMAGEBUILDER_API which can
cause fixCoreReferences to miss prefixed type refs; update the ALPHA_CORE_API
configuration's coreRef to match IMAGEBUILDER_API's
'core_v1beta1_openapi_yaml_components_schemas' (or make both the same canonical
string) so the fixCoreReferences step and any future openapi-typescript-codegen
outputs will be transformed consistently; locate the ALPHA_CORE_API object and
change its coreRef field accordingly and verify fixCoreReferences uses that same
key when normalizing imports.
| const [expandedItems, setExpandedItems] = React.useState<Record<number, boolean>>(() => | ||
| items.reduce<Record<number, boolean>>((acc, item) => { | ||
| acc[item.index] = true; | ||
| return acc; | ||
| }, {}), | ||
| ); | ||
|
|
||
| // Keep expanded state in sync when items are added/removed | ||
| React.useEffect(() => { | ||
| setExpandedItems((prev) => { | ||
| const next = { ...prev }; | ||
| items.forEach((item) => { | ||
| if (next[item.index] === undefined) next[item.index] = true; | ||
| }); | ||
| return next; | ||
| }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [items.length]); |
There was a problem hiding this comment.
Stale expandedItems keys after item removal can corrupt expand state for re-added items.
expandedItems is keyed by item.index (positional integer). When an item is removed, rjsf shifts indices for remaining items, but the stale key-value pairs from the removed item's index linger in state. The useEffect at line 168 only adds missing keys (for newly added items) but never removes stale ones or re-maps them. If a user removes item at index 0 and then adds a new item, that new item at index 0 will inherit the previous item's expansion state via the stale key.
Consider using item.key (rjsf's stable string key) instead of item.index as the state map key, and cleaning up keys that no longer exist:
♻️ Proposed fix
- const [expandedItems, setExpandedItems] = React.useState<Record<number, boolean>>(() =>
- items.reduce<Record<number, boolean>>((acc, item) => {
- acc[item.index] = true;
+ const [expandedItems, setExpandedItems] = React.useState<Record<string, boolean>>(() =>
+ items.reduce<Record<string, boolean>>((acc, item) => {
+ acc[item.key] = true;
return acc;
}, {}),
);
// Keep expanded state in sync when items are added/removed
React.useEffect(() => {
setExpandedItems((prev) => {
- const next = { ...prev };
- items.forEach((item) => {
- if (next[item.index] === undefined) next[item.index] = true;
- });
- return next;
+ const activeKeys = new Set(items.map((item) => item.key));
+ const next: Record<string, boolean> = {};
+ items.forEach((item) => {
+ next[item.key] = prev[item.key] ?? true;
+ });
+ return next;
});
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [items.length]);And update the toggle/expand references accordingly:
- isExpanded={expandedItems[item.index] !== false}
+ isExpanded={expandedItems[item.key] !== false}
onToggle={(_, expanded) => {
- setExpandedItems((prev) => ({ ...prev, [item.index]: expanded }));
+ setExpandedItems((prev) => ({ ...prev, [item.key]: expanded }));
}}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/DynamicForm/FieldTemplate.tsx` around lines
160 - 177, The expandedItems state currently uses item.index which becomes stale
when array items are removed/shifted; change the state map to use rjsf's stable
item.key instead and update all places that read/write expandedItems (including
the toggle handler and any expand checks) to use item.key; initialize
expandedItems by reducing items.map(i => i.key) to true and modify the
React.useEffect to rebuild/merge state by iterating current items and producing
a new object that keeps existing values for existing item.key entries and drops
any keys that no longer exist (so removed items are cleaned up), then call
setExpandedItems with that cleaned map.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
libs/ui-components/src/components/Catalog/CatalogPage.tsx (1)
189-189:⚠️ Potential issue | 🟡 MinorFilter empty check is incorrect for array type.
!catalogFilter.itemTypechecks for undefined/null but not an empty array. Based on the usage inCatalogPageFilter,itemTypeis an array, so an empty array[]would still be truthy.- const filterIsEmpty = !catalogFilter.itemType; + const filterIsEmpty = catalogFilter.itemType.length === 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx` at line 189, filterIsEmpty currently uses "!catalogFilter.itemType" which misses empty arrays; update the check in CatalogPage (symbol: filterIsEmpty) to treat catalogFilter.itemType (from CatalogPageFilter) as empty when it's an array with length 0 — e.g. use Array.isArray(catalogFilter.itemType) ? catalogFilter.itemType.length === 0 : !catalogFilter.itemType — so empty arrays are considered empty filters.
🧹 Nitpick comments (10)
libs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsx (1)
22-35: ReuseUpdateSuccessPageContentto remove duplicated empty-state markup.Both components duplicate title/body rendering. Compose
UpdateSuccessPagewithUpdateSuccessPageContentso message/title changes stay in one place.Also applies to: 45-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsx` around lines 22 - 35, The UpdateSuccessPage duplicates the same EmptyState title/body markup as UpdateSuccessPageContent; refactor UpdateSuccessPage to render UpdateSuccessPageContent (pass isDevice and children) instead of duplicating the EmptyState JSX so title/text changes are centralized in UpdateSuccessPageContent, and remove the duplicated EmptyState markup in UpdateSuccessPage (also apply the same change for the other similar component instance that mirrors this markup).libs/ui-components/src/components/Catalog/EditWizard/EditAppWizard.tsx (1)
154-160: Consider using an Error object instead of throwing a translated string.Throwing a plain string works but using an
Errorobject is more idiomatic and preserves stack traces for debugging.Proposed improvement
const { onSubmit, error, schemaErrors, setError } = useSubmitCatalogForm<AppUpdateFormik>(async (values) => { const catalogItemVersion = catalogItem.spec.versions.find((v) => v.version === values.version); if (!catalogItemVersion) { - throw t('Version {{version}} not found', { version: values.version }); + throw new Error(t('Version {{version}} not found', { version: values.version })); } await onUpdate(catalogItemVersion, values); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/EditAppWizard.tsx` around lines 154 - 160, The code in EditAppWizard's useSubmitCatalogForm callback throws a translated string when the requested version isn't found; replace that with an Error object to preserve stack traces and be idiomatic (in the block inside useSubmitCatalogForm<AppUpdateFormik>(...) where catalogItemVersion is checked, change throw t('Version {{version}} not found', { version: values.version }) to throw new Error(t('Version {{version}} not found', { version: values.version })) so onSubmit/error handlers receive an Error instance).libs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsx (2)
20-27: Unusederrorsparameter in validation function.The
errorsparameter is declared but never used invalidateUpdateWizardStep. Consider removing it if validation doesn't depend on Formik errors, or document why it's kept for future use.✏️ Suggested fix
const validateUpdateWizardStep = ( activeStepId: string, - errors: FormikErrors<InstallSpecFormik>, + _errors: FormikErrors<InstallSpecFormik>, values: InstallSpecFormik, ) => { if (activeStepId === versionStepId) return !!values.version; return true; };If the signature must match
FlightCtlWizardFooter'svalidateStepcallback type, prefix with underscore to indicate intentional non-use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsx` around lines 20 - 27, The validateUpdateWizardStep function currently declares an unused errors parameter; update its signature to either remove the errors parameter (if not required) or rename it to _errors to indicate intentional non-use so it satisfies FlightCtlWizardFooter's validateStep callback type; touch the function validateUpdateWizardStep and any callers expecting that exact signature and ensure versionStepId and InstallSpecFormik usage remain unchanged.
77-86: Typo and unused prop:exisingLabelsshould beexistingLabelsand appears unused.The prop name has a typo (
exisingLabels→existingLabels), and this prop is not used within the component body. If the prop is needed for future use, consider adding a TODO comment; otherwise, remove it.✏️ Suggested fix
type EditOsWizardProps = { catalogItem: CatalogItem; currentVersion: CatalogItemVersion; onUpdate: (catalogItemVersion: CatalogItemVersion, values: InstallSpecFormik) => Promise<void>; currentChannel: string; - exisingLabels: Record<string, string> | undefined; + existingLabels?: Record<string, string>; isEdit: boolean; version: string; channel: string; };If the prop is intentionally unused for now, document it or remove it from the interface.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsx` around lines 77 - 86, EditOsWizardProps declares a misspelled and unused prop "exisingLabels"; rename it to "existingLabels" in the EditOsWizardProps type and update all usages (props destructuring in the EditOsWizard component and any callers) to the corrected name, or if it's intentionally unused remove "exisingLabels" entirely from EditOsWizardProps and from any function/component signatures and callers; if you keep it but don't use it yet, add a short TODO comment above the prop in EditOsWizardProps indicating intended future use to avoid linter distractions.libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx (1)
100-122: Typo in prop name:exisingLabelsshould beexistingLabels.Line 106 passes
exisingLabelswhich appears to be a typo. This should beexistingLabelsfor consistency and clarity.✏️ Suggested fix
content = ( <EditOsWizard isEdit={!version} catalogItem={catalogItem} currentChannel={currentChannel} currentVersion={currentVersion} - exisingLabels={currentLabels} + existingLabels={currentLabels} version={version} channel={channel}Note: This requires updating the
EditOsWizardcomponent's prop type definition as well (exisingLabels→existingLabelsinEditOsWizardProps).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx` around lines 100 - 122, The prop name passed to EditOsWizard is misspelled as exisingLabels; change it to existingLabels in the JSX (within EditWizard where EditOsWizard is rendered) and update the EditOsWizard component's prop interface/type (e.g., EditOsWizardProps) to rename exisingLabels → existingLabels, then search for any other references/usages of exisingLabels in the codebase and update them to existingLabels to keep types and consumers consistent.libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx (2)
122-127: Avoid returning values fromforEachcallbacks.The static analysis tool flags that the
forEachcallback implicitly returns the result ofMap.set(). While functionally harmless, use explicit block syntax for clarity.✏️ Suggested fix
// Deduplicate entries const entriesMap = new Map<string, CatalogItemVersion>(); - directUpgradeEntries.forEach((entry) => entriesMap.set(entry.version, entry)); + directUpgradeEntries.forEach((entry) => { + entriesMap.set(entry.version, entry); + }); if (currentVersionEntry) { entriesMap.set(currentVersionEntry.version, currentVersionEntry); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx` around lines 122 - 127, The forEach callback on directUpgradeEntries implicitly returns Map.set(), triggering the static analysis warning; change the callback to an explicit block-style arrow function or replace forEach with a for..of loop so the Map is updated without an implicit return—modify the code that constructs entriesMap (using entriesMap, directUpgradeEntries, currentVersionEntry, allEntries) to call entriesMap.set(...) inside a statement block (e.g., directUpgradeEntries.forEach(entry => { entriesMap.set(...); })) or use for (const entry of directUpgradeEntries) { entriesMap.set(...); }.
92-103: Type assertionas neversuggests a type compatibility issue.The
as nevercast on line 97 is a type escape hatch. This may indicate a mismatch betweenwithSelection()HOC's return type and whatComponentFactoryexpects. Consider investigating the proper typing or adding a comment explaining why this is necessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx` around lines 92 - 103, The switch in customComponentFactory is masking a real type mismatch by using the `as never` cast on VersionNode; remove the `as never` and fix the types so VersionNode satisfies ComponentFactory's expected return type. Inspect the ComponentFactory return signature and the props/ComponentType of VersionNode (and any HOC wrappers like withPanZoom()/withSelection()) and either (a) adjust VersionNode's exported component type/props to match ComponentFactory, (b) wrap VersionNode with the proper HOC that yields the expected ComponentFactory type, or (c) use a precise cast to the correct ComponentFactory return type (not never) so the compiler understands the component is compatible. Ensure changes reference customComponentFactory, ComponentFactory, ModelKind, and VersionNode so future reviewers can find and verify the fix.libs/ui-components/src/components/Catalog/InstalledSoftware.tsx (3)
251-251: Unnecessary fallback formetadata.nameon API-returned resources.Per coding guidelines,
resource.metadata.nameis always defined for resources returned from the API. The|| ''fallbacks are defensive but unnecessary here sinceosItemandapp.itemare fetched from the API.Also applies to: 260-260, 286-286, 327-327
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` at line 251, Remove the unnecessary defensive fallbacks for metadata.name on API-returned resources: replace usages like osItem.metadata.name || '' and app.item.metadata.name || '' with the direct property (osItem.metadata.name and app.item.metadata.name) in the onEdit/onEditOs/onEditApp calls and any similar callbacks (e.g., the lines using onEditOs, onEditApp, and wherever osItem or app.item metadata is passed); ensure the props/functions onEdit and related handlers accept the non-null string as before and adjust no other logic.
48-61: Consider consolidatingUpdateAppColumnandUpdateOsColumninto a single component.These two components are nearly identical, differing only in the callback prop name (
onEditAppvsonEditOs). This is a minor duplication that could be unified.♻️ Optional: Unified component
-type UpdateColumnProps = { - catalogItem: CatalogItem; - channel: string; - catalogItemVersion: CatalogItemVersion; - appName: string | undefined; - onEditApp: VoidFunction; -}; - -const UpdateAppColumn = ({ catalogItem, channel, catalogItemVersion, onEditApp }: UpdateColumnProps) => { - const { t } = useTranslation(); - const updates = getUpdates(catalogItem, channel, catalogItemVersion.version); - - return ( - <> - {!!updates.length && ( - <Button variant="link" isInline onClick={onEditApp} icon={<ArrowCircleUpIcon />}> - {t('Update available')} - </Button> - )} - </> - ); -}; - -type UpdateOsColumnProps = { - catalogItem: CatalogItem; - channel: string; - catalogItemVersion: CatalogItemVersion; - onEditOs: VoidFunction; -}; - -const UpdateOsColumn = ({ onEditOs, catalogItem, channel, catalogItemVersion }: UpdateOsColumnProps) => { - const { t } = useTranslation(); - const updates = getUpdates(catalogItem, channel, catalogItemVersion.version); - - return ( - <> - {!!updates.length && ( - <Button variant="link" isInline onClick={onEditOs} icon={<ArrowCircleUpIcon />}> - {t('Update available')} - </Button> - )} - </> - ); -}; +type UpdateColumnProps = { + catalogItem: CatalogItem; + channel: string; + catalogItemVersion: CatalogItemVersion; + onEdit: VoidFunction; +}; + +const UpdateColumn = ({ catalogItem, channel, catalogItemVersion, onEdit }: UpdateColumnProps) => { + const { t } = useTranslation(); + const updates = getUpdates(catalogItem, channel, catalogItemVersion.version); + + return ( + <> + {!!updates.length && ( + <Button variant="link" isInline onClick={onEdit} icon={<ArrowCircleUpIcon />}> + {t('Update available')} + </Button> + )} + </> + ); +};Also applies to: 70-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 48 - 61, Two near-duplicate components UpdateAppColumn and UpdateOsColumn should be consolidated: create a single GenericUpdateColumn (or UpdateColumn) that accepts props matching UpdateColumnProps but with a generic onEdit callback (e.g., onEdit: () => void) instead of onEditApp/onEditOs; reuse useTranslation, getUpdates(catalogItem, channel, catalogItemVersion.version), and the ArrowCircleUpIcon/Button rendering logic inside that component, then replace usages of UpdateAppColumn and UpdateOsColumn with the unified component passing the appropriate onEdit handler from callers.
179-199: Effect may trigger unnecessary fetches whenappsarray reference changes.The
appsdependency fromuseMemocreates a new array reference on each render (even if contents are the same), which could cause redundant API calls. Consider adding an early return or stabilizing the dependency.♻️ Proposed optimization
React.useEffect(() => { + if (apps.length === 0) { + setAppItems([]); + setAppsLoading(false); + return; + } (async () => { const appRequests = apps.map((app) => get<CatalogItem>(`catalogs/${app.catalog}/items/${app.item}`)); const results = await Promise.allSettled(appRequests);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx` around lines 179 - 199, The effect in InstalledSoftware re-fetches whenever the apps array reference changes; to avoid redundant calls, add a stability guard inside the useEffect: keep a useRef(prevApps) and perform a shallow/deep equality check (compare key fields like catalog,item,name or JSON.stringify) between prevApps.current and apps, and early-return if they are equal; only update prevApps.current and proceed to run the async fetch when the contents actually differ, then setAppItems and setAppsLoading as before. Reference the InstalledSoftware component, the useEffect block, the apps dependency, and the setAppItems/setAppsLoading state updates when implementing this guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx`:
- Around line 218-235: The key for CatalogItemCard uses only ci.metadata.name
which may collide across catalogs; update the key to include the catalog
identifier so it is globally unique (e.g., combine ci.metadata.catalog and
ci.metadata.name) and ensure any related selection logic (setSelectedItem usage)
still relies on the same composite identity (catalog + itemName) such that
CatalogItemCard keys and selection state are consistent.
In `@libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx`:
- Around line 235-261: The controller is currently created inside a
React.useMemo keyed only by currentChannel, causing stale visuals when
currentVersion or updates change; update the logic so the Visualization
controller still memoizes on creation (Visualization, customComponentFactory,
customLayoutFactory) but apply model updates in a React.useEffect that rebuilds
the model via buildTopologyModel(currentVersion, updates, currentChannel) and
calls controller.fromModel(model, false) whenever controller, currentVersion,
updates, or currentChannel change; keep existing event listeners
(GRAPH_LAYOUT_END_EVENT, SELECTION_EVENT) on the Visualization instance and
remove currentVersion/updates from the stale dependency gap.
In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx`:
- Around line 282-293: The actions array in InstalledSoftware.tsx can contain an
empty object when itemVersion is falsy, which may render a broken ActionsColumn
entry; change the construction of actions (the IAction[] containing the
conditional edit action and the delete action) so the edit action is only added
when itemVersion is truthy — e.g. build actions by conditionally spreading the
edit action (or push it only when itemVersion) or filter the array to remove
empty objects (e.g. .filter(Boolean) or .filter(a => Object.keys(a).length))
before passing to ActionsColumn; ensure references to onEdit and setAppToDelete
remain unchanged.
In
`@libs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsx`:
- Around line 68-72: The navigate call in UpdateSuccessPage.tsx currently passes
postfix using optional chaining (device?.metadata.name / fleet?.metadata.name)
and can navigate to invalid routes; update the logic that handles target to
explicitly check target === 'device' or 'fleet' and verify the corresponding
resource exists (device for device path, fleet for fleet path) before calling
navigate, and only call navigate with ROUTE.DEVICE_DETAILS and
device.metadata.name or ROUTE.FLEET_DETAILS and fleet.metadata.name when the
resource is non-null; otherwise skip navigation or route to a safe fallback.
Ensure you modify the block containing the navigate(...) invocation so that
target, device, and fleet are guarded and no undefined postfix is ever passed.
---
Duplicate comments:
In `@libs/ui-components/src/components/Catalog/CatalogPage.tsx`:
- Line 189: filterIsEmpty currently uses "!catalogFilter.itemType" which misses
empty arrays; update the check in CatalogPage (symbol: filterIsEmpty) to treat
catalogFilter.itemType (from CatalogPageFilter) as empty when it's an array with
length 0 — e.g. use Array.isArray(catalogFilter.itemType) ?
catalogFilter.itemType.length === 0 : !catalogFilter.itemType — so empty arrays
are considered empty filters.
---
Nitpick comments:
In `@libs/ui-components/src/components/Catalog/EditWizard/EditAppWizard.tsx`:
- Around line 154-160: The code in EditAppWizard's useSubmitCatalogForm callback
throws a translated string when the requested version isn't found; replace that
with an Error object to preserve stack traces and be idiomatic (in the block
inside useSubmitCatalogForm<AppUpdateFormik>(...) where catalogItemVersion is
checked, change throw t('Version {{version}} not found', { version:
values.version }) to throw new Error(t('Version {{version}} not found', {
version: values.version })) so onSubmit/error handlers receive an Error
instance).
In `@libs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsx`:
- Around line 20-27: The validateUpdateWizardStep function currently declares an
unused errors parameter; update its signature to either remove the errors
parameter (if not required) or rename it to _errors to indicate intentional
non-use so it satisfies FlightCtlWizardFooter's validateStep callback type;
touch the function validateUpdateWizardStep and any callers expecting that exact
signature and ensure versionStepId and InstallSpecFormik usage remain unchanged.
- Around line 77-86: EditOsWizardProps declares a misspelled and unused prop
"exisingLabels"; rename it to "existingLabels" in the EditOsWizardProps type and
update all usages (props destructuring in the EditOsWizard component and any
callers) to the corrected name, or if it's intentionally unused remove
"exisingLabels" entirely from EditOsWizardProps and from any function/component
signatures and callers; if you keep it but don't use it yet, add a short TODO
comment above the prop in EditOsWizardProps indicating intended future use to
avoid linter distractions.
In `@libs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsx`:
- Around line 100-122: The prop name passed to EditOsWizard is misspelled as
exisingLabels; change it to existingLabels in the JSX (within EditWizard where
EditOsWizard is rendered) and update the EditOsWizard component's prop
interface/type (e.g., EditOsWizardProps) to rename exisingLabels →
existingLabels, then search for any other references/usages of exisingLabels in
the codebase and update them to existingLabels to keep types and consumers
consistent.
In `@libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx`:
- Around line 122-127: The forEach callback on directUpgradeEntries implicitly
returns Map.set(), triggering the static analysis warning; change the callback
to an explicit block-style arrow function or replace forEach with a for..of loop
so the Map is updated without an implicit return—modify the code that constructs
entriesMap (using entriesMap, directUpgradeEntries, currentVersionEntry,
allEntries) to call entriesMap.set(...) inside a statement block (e.g.,
directUpgradeEntries.forEach(entry => { entriesMap.set(...); })) or use for
(const entry of directUpgradeEntries) { entriesMap.set(...); }.
- Around line 92-103: The switch in customComponentFactory is masking a real
type mismatch by using the `as never` cast on VersionNode; remove the `as never`
and fix the types so VersionNode satisfies ComponentFactory's expected return
type. Inspect the ComponentFactory return signature and the props/ComponentType
of VersionNode (and any HOC wrappers like withPanZoom()/withSelection()) and
either (a) adjust VersionNode's exported component type/props to match
ComponentFactory, (b) wrap VersionNode with the proper HOC that yields the
expected ComponentFactory type, or (c) use a precise cast to the correct
ComponentFactory return type (not never) so the compiler understands the
component is compatible. Ensure changes reference customComponentFactory,
ComponentFactory, ModelKind, and VersionNode so future reviewers can find and
verify the fix.
In `@libs/ui-components/src/components/Catalog/InstalledSoftware.tsx`:
- Line 251: Remove the unnecessary defensive fallbacks for metadata.name on
API-returned resources: replace usages like osItem.metadata.name || '' and
app.item.metadata.name || '' with the direct property (osItem.metadata.name and
app.item.metadata.name) in the onEdit/onEditOs/onEditApp calls and any similar
callbacks (e.g., the lines using onEditOs, onEditApp, and wherever osItem or
app.item metadata is passed); ensure the props/functions onEdit and related
handlers accept the non-null string as before and adjust no other logic.
- Around line 48-61: Two near-duplicate components UpdateAppColumn and
UpdateOsColumn should be consolidated: create a single GenericUpdateColumn (or
UpdateColumn) that accepts props matching UpdateColumnProps but with a generic
onEdit callback (e.g., onEdit: () => void) instead of onEditApp/onEditOs; reuse
useTranslation, getUpdates(catalogItem, channel, catalogItemVersion.version),
and the ArrowCircleUpIcon/Button rendering logic inside that component, then
replace usages of UpdateAppColumn and UpdateOsColumn with the unified component
passing the appropriate onEdit handler from callers.
- Around line 179-199: The effect in InstalledSoftware re-fetches whenever the
apps array reference changes; to avoid redundant calls, add a stability guard
inside the useEffect: keep a useRef(prevApps) and perform a shallow/deep
equality check (compare key fields like catalog,item,name or JSON.stringify)
between prevApps.current and apps, and early-return if they are equal; only
update prevApps.current and proceed to run the async fetch when the contents
actually differ, then setAppItems and setAppsLoading as before. Reference the
InstalledSoftware component, the useEffect block, the apps dependency, and the
setAppItems/setAppsLoading state updates when implementing this guard.
In
`@libs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsx`:
- Around line 22-35: The UpdateSuccessPage duplicates the same EmptyState
title/body markup as UpdateSuccessPageContent; refactor UpdateSuccessPage to
render UpdateSuccessPageContent (pass isDevice and children) instead of
duplicating the EmptyState JSX so title/text changes are centralized in
UpdateSuccessPageContent, and remove the duplicated EmptyState markup in
UpdateSuccessPage (also apply the same change for the other similar component
instance that mirrors this markup).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/ocp-plugin/console-extensions.jsonapps/ocp-plugin/package.jsonapps/ocp-plugin/src/components/Catalog/CatalogEditDeviceWizard.tsxapps/ocp-plugin/src/components/Catalog/CatalogEditFleetWizard.tsxapps/standalone/src/app/routes.tsxlibs/i18n/locales/en/translation.jsonlibs/ui-components/src/components/Catalog/CatalogPage.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditAppWizard.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditOsWizard.tsxlibs/ui-components/src/components/Catalog/EditWizard/EditWizard.tsxlibs/ui-components/src/components/Catalog/EditWizard/steps/ReviewStep.tsxlibs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.csslibs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsxlibs/ui-components/src/components/Catalog/EditWizard/steps/UpdateStep.tsxlibs/ui-components/src/components/Catalog/EditWizard/types.tslibs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsxlibs/ui-components/src/components/Catalog/InstallWizard/steps/AppConfigStep.tsxlibs/ui-components/src/components/Catalog/InstalledSoftware.tsxlibs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsCatalog.tsxlibs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsCatalog.tsxlibs/ui-components/src/hooks/useAppContext.tsxlibs/ui-components/src/hooks/useNavigate.tsx
✅ Files skipped from review due to trivial changes (1)
- libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.css
🚧 Files skipped from review as they are similar to previous changes (7)
- libs/ui-components/src/components/Catalog/ResourceCatalog/ResourceCatalogPage.tsx
- libs/ui-components/src/hooks/useNavigate.tsx
- libs/ui-components/src/hooks/useAppContext.tsx
- apps/ocp-plugin/console-extensions.json
- apps/ocp-plugin/package.json
- apps/standalone/src/app/routes.tsx
- libs/ui-components/src/components/Fleet/FleetDetails/FleetDetailsCatalog.tsx
| const controller = React.useMemo(() => { | ||
| const newController = new Visualization(); | ||
| newController.registerComponentFactory(customComponentFactory); | ||
| newController.registerLayoutFactory(customLayoutFactory); | ||
| newController.addEventListener(GRAPH_LAYOUT_END_EVENT, () => { | ||
| newController.getGraph().fit(80); | ||
| }); | ||
| newController.addEventListener(SELECTION_EVENT, (ids: string[]) => { | ||
| const selectedId = ids[0]; | ||
| if (selectedId) { | ||
| const node = newController.getNodeById(selectedId); | ||
| if (node) { | ||
| const data = node.getData() as VersionNodeData | undefined; | ||
| if (data?.entryName) { | ||
| onSelectionChange(selectedId, data.entryName); | ||
| } | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| const model = buildTopologyModel(currentVersion, updates, currentChannel); | ||
| newController.fromModel(model, false); | ||
|
|
||
| return newController; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [currentChannel]); | ||
|
|
There was a problem hiding this comment.
useMemo dependencies may cause stale graph when currentVersion or updates change.
The controller is memoized only on currentChannel, but buildTopologyModel also depends on currentVersion and updates. If these props change without currentChannel changing, the graph won't rebuild with the new data.
🔧 Suggested fix
const model = buildTopologyModel(currentVersion, updates, currentChannel);
newController.fromModel(model, false);
return newController;
- // eslint-disable-next-line react-hooks/exhaustive-deps
- }, [currentChannel]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [currentChannel, currentVersion, updates]);Alternatively, if recreating the controller is expensive, consider using a useEffect to update the model when currentVersion or updates change:
React.useEffect(() => {
const model = buildTopologyModel(currentVersion, updates, currentChannel);
controller.fromModel(model, false);
}, [controller, currentVersion, updates, currentChannel]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const controller = React.useMemo(() => { | |
| const newController = new Visualization(); | |
| newController.registerComponentFactory(customComponentFactory); | |
| newController.registerLayoutFactory(customLayoutFactory); | |
| newController.addEventListener(GRAPH_LAYOUT_END_EVENT, () => { | |
| newController.getGraph().fit(80); | |
| }); | |
| newController.addEventListener(SELECTION_EVENT, (ids: string[]) => { | |
| const selectedId = ids[0]; | |
| if (selectedId) { | |
| const node = newController.getNodeById(selectedId); | |
| if (node) { | |
| const data = node.getData() as VersionNodeData | undefined; | |
| if (data?.entryName) { | |
| onSelectionChange(selectedId, data.entryName); | |
| } | |
| } | |
| } | |
| }); | |
| const model = buildTopologyModel(currentVersion, updates, currentChannel); | |
| newController.fromModel(model, false); | |
| return newController; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [currentChannel]); | |
| const controller = React.useMemo(() => { | |
| const newController = new Visualization(); | |
| newController.registerComponentFactory(customComponentFactory); | |
| newController.registerLayoutFactory(customLayoutFactory); | |
| newController.addEventListener(GRAPH_LAYOUT_END_EVENT, () => { | |
| newController.getGraph().fit(80); | |
| }); | |
| newController.addEventListener(SELECTION_EVENT, (ids: string[]) => { | |
| const selectedId = ids[0]; | |
| if (selectedId) { | |
| const node = newController.getNodeById(selectedId); | |
| if (node) { | |
| const data = node.getData() as VersionNodeData | undefined; | |
| if (data?.entryName) { | |
| onSelectionChange(selectedId, data.entryName); | |
| } | |
| } | |
| } | |
| }); | |
| const model = buildTopologyModel(currentVersion, updates, currentChannel); | |
| newController.fromModel(model, false); | |
| return newController; | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [currentChannel, currentVersion, updates]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/ui-components/src/components/Catalog/EditWizard/steps/UpdateGraph.tsx`
around lines 235 - 261, The controller is currently created inside a
React.useMemo keyed only by currentChannel, causing stale visuals when
currentVersion or updates change; update the logic so the Visualization
controller still memoizes on creation (Visualization, customComponentFactory,
customLayoutFactory) but apply model updates in a React.useEffect that rebuilds
the model via buildTopologyModel(currentVersion, updates, currentChannel) and
calls controller.fromModel(model, false) whenever controller, currentVersion,
updates, or currentChannel change; keep existing event listeners
(GRAPH_LAYOUT_END_EVENT, SELECTION_EVENT) on the Visualization instance and
remove currentVersion/updates from the stale dependency gap.
libs/ui-components/src/components/Catalog/InstallWizard/UpdateSuccessPage.tsx
Outdated
Show resolved
Hide resolved
rawagner
force-pushed
the
catalog
branch
6 times, most recently
from
466ef1a to
d5db25b
Compare
| ), | ||
| }, | ||
| { | ||
| path: 'catalog/:fleetId/:catalogId/:itemId', |
There was a problem hiding this comment.
The pattern here is a bit weird as for all the other fleet routes we have
fleet/<fleetId>/subpath
I assume this is necessary because we want to have this page not go via the usual FleetDetails route, so it's fine.
| </Icon> | ||
| </GridItem> | ||
| <GridItem span={5}> | ||
| {updates.length ? ( |
There was a problem hiding this comment.
Can be done later, but I think it should be possible for users to rollback to a previous version.
Now, if they're at the latest version, then they just see "Up to date" without an option to change it.
| } | ||
|
|
||
| if (categories.length === 1) { | ||
| parts.push(`spec.category==${categories[0]}`); |
There was a problem hiding this comment.
Nit: I think it should work with the plural form even if only one category is selected, and same for the other filters.
flightctl get catalogitems --field-selector="spec.category in (aplication)"
(cherry picked from commit 65deb96)
2 participants
Summary by CodeRabbit
New Features
Enhancements