EDM-2611: UI for Image building to quickly enroll new devices #455
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds end-to-end Image Build support: generated imagebuilder OpenAPI types and generation tooling, new frontend pages/components/wizard/hooks/contexts for ImageBuild/ImageExport, routing and proxy wiring to an imagebuilder backend, repository/OCI integrations, translations, and related UI/type adjustments. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
celdrake
force-pushed
the
EDM-2611-image-building
branch
7 times, most recently
from
3087310 to
8e616a6
Compare
celdrake
force-pushed
the
EDM-2611-image-building
branch
from
8e616a6 to
5135c9c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/ui-components/src/components/form/FormSelect.tsx (1)
40-45: Avoid auto-selecting a disabled-only option.Now that per-item
isDisabledexists, the single-item auto-select (Line 40-45) can pick an option that should be unselectable. Consider skipping auto-select when the lone item is disabled.🛠️ Suggested fix
React.useEffect(() => { const hasOneItem = itemKeys.length === 1; if (hasOneItem && !field.value) { - setValue(itemKeys[0], true); + const onlyItem = items[itemKeys[0]]; + const onlyItemDisabled = isItemObject(onlyItem) && onlyItem.isDisabled; + if (!onlyItemDisabled) { + setValue(itemKeys[0], true); + } } -}, [itemKeys, field.value, setValue]); +}, [itemKeys, field.value, setValue, items]);Also applies to: 100-105
libs/ui-components/src/components/common/CodeEditor/YamlEditor.tsx (1)
20-59: Handle ImageBuild in getResourceEndpoint.ImageBuild is now a supported YAML resource type, but attempting to save one will throw because the endpoint switch doesn't include a case for it. Even though ImageBuildYaml disables editing by default, the type signature allows ImageBuild to be passed to YamlEditor, creating a type-runtime mismatch.
🐛 Proposed fix
switch (kind) { case ResourceKind.FLEET: return `fleets/${resourceName}`; case ResourceKind.DEVICE: return `devices/${resourceName}`; case ResourceKind.REPOSITORY: return `repositories/${resourceName}`; case ResourceKind.AUTH_PROVIDER: return `authproviders/${resourceName}`; + case ResourceKind.IMAGE_BUILD: + return `imagebuilds/${resourceName}`; default: throw new Error(`Unsupported resource kind: ${kind}`); }
🤖 Fix all issues with AI agents
In `@libs/types/scripts/openapi-typescript.js`:
- Around line 49-52: The fetch result for swaggerUrl isn't validated; update the
code after the fetch (where response is assigned) to check response.ok and throw
or log a clear error containing response.status and response.statusText along
with swaggerUrl and mode before attempting response.text(), so that non-2xx
responses (e.g., 404) produce an immediate, informative failure; ensure the
thrown/logged message references response, swaggerUrl and mode to aid debugging.
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/OutputImageStep.tsx`:
- Around line 90-95: Fix the minor punctuation in the helper text inside
OutputImageStep: in the TextField with name "destination.tag" (helperText prop),
change "Specify the version (e.g, latest or 9.6)" to use correct punctuation —
e.g., "Specify the version (e.g., latest or 9.6)". Ensure you update the
helperText string in the OutputImageStep component where FormGroup and TextField
are defined.
- Around line 47-57: The handler assumes values.exportFormats is always defined,
which can cause runtime errors; in handleFormatToggle (and the similar logic
around lines 105-125) read a local fallback like const currentFormats =
values.exportFormats || [] before using spread, filter, includes, or length,
then call setFieldValue with that safe array (when adding,
setFieldValue('exportFormats', [...currentFormats, format]); when removing,
setFieldValue('exportFormats', currentFormats.filter(f => f !== format))); also
ensure any other checks (e.g., includes/length) reference the same fallback
array.
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/RegistrationStep.tsx`:
- Around line 35-36: The late-binding radio/checkbox should be checked by
explicitly comparing values.bindingType to BindingType.BindingTypeLate rather
than using !isEarlyBindingSelected; update the logic where
isEarlyBindingSelected is used (and the corresponding checked prop between lines
around 35 and 120-125) to use an explicit comparison (e.g., const
isLateBindingSelected = values.bindingType === BindingType.BindingTypeLate) and
set the checked prop based on that explicit boolean so the late option is not
shown selected when bindingType is unset.
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/ReviewStep.tsx`:
- Around line 185-206: Remove the redundant nested StackItem wrapper around the
Alert in the ReviewStep component: locate the JSX block where a StackItem
contains another StackItem (the inner one wrapping the Alert and export error
List generated from error.errors.map) and remove the inner StackItem so the
Alert is a direct child of the outer StackItem; keep the Alert, Content, List,
and ListItem mapping (including getExportFormatLabel and getErrorMessage usage)
intact to preserve behavior and keys.
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/SourceImageStep.tsx`:
- Around line 19-25: The isSourceImageStepValid function currently treats any
truthy errors.source as potentially valid; update it so the step is valid only
when there are no source errors at all: if errors.source is falsy return true;
if errors.source is a string (parent-level error) return false; otherwise (when
source is an object) return true only if source.repository, source.imageName,
and source.imageTag are all falsy. Use the isSourceImageStepValid function and
the errors.source, repository, imageName, imageTag identifiers to locate and
implement this logic.
In
`@libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/useEditImageBuild.ts`:
- Around line 14-34: The effect in useEditImageBuild.ts currently skips refetch
when imageBuild is already set, causing stale data; update the React.useEffect
to always fetch whenever imageBuildId changes: remove the "!imageBuild" guard so
the effect runs on imageBuildId truthy, and when imageBuildId becomes falsy
reset imageBuild, error, and isLoading state. Add request cancellation using an
AbortController (or similar) inside the effect to cancel previous in-flight get
requests and avoid race conditions; pass the abort signal to get, ignore
operations when aborted, and ensure setIsLoading(true) before the fetch and
setIsLoading(false) in finally. Keep dependencies (imageBuildId, get) correct
and update setImageBuild via toImageBuildWithExports on success and setError on
failure.
In
`@libs/ui-components/src/components/ImageBuilds/DeleteImageBuildModal/DeleteImageBuildModal.tsx`:
- Around line 60-76: The delete button is currently disabled when an error
exists so users cannot retry; update the Button used in DeleteImageBuildModal
(the Button with key="confirm") to only disable based on isDeleting (remove the
!!error check from isDisabled) so retries are allowed, keep the existing
setError(undefined) at the start of the onClick handler to clear prior errors,
and leave the error handling in the catch
(setError(getErrorMessage(imageBuildErr)); setIsDeleting(false)) and success
path (onClose(true)) unchanged.
In `@libs/ui-components/src/components/ImageBuilds/ImageBuildAndExportStatus.tsx`:
- Around line 97-99: Remove the no-op self-assignment in the
ImageBuildAndExportStatus component: delete the redundant "else if (message) {
message = message; }" branch (or the entire else-if) and keep only the
meaningful branches that set or use the message variable so there is no dead
code around the message handling logic in ImageBuildAndExportStatus.
In
`@libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx`:
- Around line 89-99: The code calls getExportDownloadResult(response) which
consumes response.blob() before checking response.ok; update the flow so you
check response.ok (and handle redirect status) before consuming the body: either
move the response.ok / response.status check above the getExportDownloadResult
call in the ImageBuildExportsGallery component (so you only call
getExportDownloadResult when response.ok or redirect), or modify
getExportDownloadResult to inspect response.ok and return an early
error/redirect result without calling response.blob() for non-ok responses;
adjust downstream logic that uses downloadResult (createDownloadLink,
showSpinnerBriefly, saveAs) accordingly so blobs are only read for successful
responses.
In `@libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx`:
- Around line 100-102: In ImageBuildRow.tsx the expanded row uses <Tr
isExpanded={isExpanded}> with a <Td colSpan={7}> which doesn't match the table's
9 columns and causes misaligned/short expanded content; update the <Td> in the
expanded row to use colSpan={9} (or compute the column count dynamically) so the
ExpandableRowContent spans the full table width, keeping the isExpanded logic
and structure intact.
In `@libs/ui-components/src/components/ImageBuilds/ImageExportCards.css`:
- Around line 10-12: The CSS variable --pf-v6-c-button--FontSize is currently
set on .fctl-imageexport-card .fctl-imageexport-card__status
.pf-v6-c-button__text which PFv6 ignores; change the selector to target the
button element (e.g. .fctl-imageexport-card .fctl-imageexport-card__status
.pf-v6-c-button) and set --pf-v6-c-button--FontSize: 0.75rem there so the
button's computed font size is applied; update the rule in ImageExportCards.css
accordingly.
In `@libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx`:
- Around line 43-54: The "View logs" button is rendered without behavior; add an
optional handler to the props and wire it or disable the button when handler is
absent. Update ImageExportFormatCardProps to include onViewLogs?: () => void,
then in the ImageExportCards/ImageExportFormatCard component attach that prop to
the button's onClick (use onViewLogs as the handler) and/or set the button
disabled when onViewLogs is undefined; also apply the same change where the
button is rendered around the lines referenced (the render block at ~174-177) so
the button either invokes onViewLogs or is disabled until the logs view exists.
In
`@libs/ui-components/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx`:
- Around line 92-99: The ModalHeader title string in MassDeleteImageBuildModal
currently has a stray space before the question mark; update the title passed to
ModalHeader (in MassDeleteImageBuildModal) from t('Delete image builds ?') to
t('Delete image builds?') so the question mark is adjacent to the last word (and
adjust any i18n key if necessary).
- Around line 70-88: The deleteImageBuilds handler leaves previous errors
visible when a retry runs—clear the stale errors at the start of the function by
calling setErrors([]) before setting progress/isDeleting; keep the rest of the
logic (mapping imageBuilds to promises, setProgressTotal(promises.length), await
Promise.allSettled, filtering with isPromiseRejected and calling
onDeleteSuccess) unchanged so new attempts start with a fresh error state.
In `@libs/ui-components/src/hooks/useNavigate.tsx`:
- Around line 32-35: The ansible appRoutes mapping in const.ts is missing the
ROUTE enum entries IMAGE_BUILDS, IMAGE_BUILD_CREATE, IMAGE_BUILD_DETAILS, and
IMAGE_BUILD_EDIT; open the appRoutes object and add keys for ROUTE.IMAGE_BUILDS,
ROUTE.IMAGE_BUILD_CREATE, ROUTE.IMAGE_BUILD_DETAILS, and ROUTE.IMAGE_BUILD_EDIT
with the appropriate path (use '/' as in other apps or the intended route),
ensuring the appRoutes object now contains all ROUTE enum values so type
coverage is satisfied.
In `@libs/ui-components/src/utils/imageBuilds.ts`:
- Around line 125-162: The getRedirectUrl function currently may return an
empty/opaque redirect (response.url) which then passes isValidDownloadUrl after
being resolved against window.location.origin; update getRedirectUrl to only
return response.url when it is non-empty and not an opaque redirect (check
response.type !== 'opaqueredirect' and response.url truthy) and keep the
existing Location header branch; this ensures getExportDownloadResult and
isValidDownloadUrl are not given an empty/opaque URL to validate and prevents
resolving to the app origin.
🧹 Nitpick comments (10)
libs/ui-components/src/components/Repository/RepositoryDetails/RepositoryGeneralDetailsCard.tsx (1)
99-99: Consider removing the redundant conditional check.The
repoDetails ?ternary appears unnecessary sincerepoDetailsis a required prop of typeRepositoryin theDetailsTabcomponent signature (line 74). The component won't render without it.Suggested simplification
- {repoDetails ? <RepositoryStatus repository={repoDetails} /> : '-'} + <RepositoryStatus repository={repoDetails} />apps/standalone/scripts/setup_env.sh (2)
66-66: Consider adding conditional service detection for the imagebuilder.Unlike
FLIGHTCTL_CLI_ARTIFACTS_SERVERandFLIGHTCTL_ALERTMANAGER_PROXYwhich are conditionally set based on service availability (lines 68-88),FLIGHTCTL_IMAGEBUILDER_SERVERis always exported. If the imagebuilder service may not always be deployed, consider adding similar conditional detection logic to avoid configuration errors when the service is unavailable.Example conditional detection pattern
+# ImageBuilder - get setting from kind cluster, unless it has been configured already +if [ -z "$ENABLE_IMAGEBUILDER" ]; then + ENABLE_IMAGEBUILDER=$(detect_service_setting "ImageBuilder" "flightctl-imagebuilder") +fi +export ENABLE_IMAGEBUILDER +if [ "$ENABLE_IMAGEBUILDER" = "true" ]; then + export FLIGHTCTL_IMAGEBUILDER_SERVER="https://$EXTERNAL_IP:8445" +else + unset FLIGHTCTL_IMAGEBUILDER_SERVER +fi -export FLIGHTCTL_IMAGEBUILDER_SERVER="https://$EXTERNAL_IP:8445"
95-95: Update the echo to handle optional/disabled state if conditional detection is added.If you adopt conditional detection for the imagebuilder service, update this line to match the pattern used for other optional services:
-echo " FLIGHTCTL_IMAGEBUILDER_SERVER=$FLIGHTCTL_IMAGEBUILDER_SERVER" >&2 +echo " FLIGHTCTL_IMAGEBUILDER_SERVER=${FLIGHTCTL_IMAGEBUILDER_SERVER:-'(disabled)'}" >&2libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildDetailsPage.tsx (3)
32-32: Initialize modal state explicitly.
useState<boolean>()without an initial value results inundefined, which requires the type to beboolean | undefined. Initialize withfalsefor cleaner typing and to avoid potential truthy/falsy edge cases.Suggested fix
- const [isDeleteModalOpen, setIsDeleteModalOpen] = React.useState<boolean>(); + const [isDeleteModalOpen, setIsDeleteModalOpen] = React.useState(false);
78-82: Remove unnecessary refetch before navigation.After a successful deletion,
refetch()is called before navigating away. Since the user immediately leaves this page, the refetch result is never displayed, making it a wasted network call.Suggested fix
onClose={(hasDeleted?: boolean) => { if (hasDeleted) { - refetch(); navigate(ROUTE.IMAGE_BUILDS); } setIsDeleteModalOpen(false); }}
73-73: Consider tracking the TODO for logs implementation.The logs tab currently renders a placeholder. Consider creating a follow-up issue to track this incomplete feature.
Would you like me to open an issue to track the logs tab implementation?
libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx (1)
54-55: Consider guarding against undefined metadata.name.The cast
imageBuild.metadata.name as stringassumes the name is always defined. If the ImageBuild was somehow created without a name (malformed data), this would passundefinedtogetImageExportResource, potentially causing downstream issues.Suggested defensive check
const handleExportImage = async (format: ExportFormatType) => { + const imageBuildName = imageBuild.metadata.name; + if (!imageBuildName) { + setError({ format, message: 'Image build name is missing', mode: 'export' }); + return; + } setExportingFormat(format); setError(null); try { const imageExport = getImageExportResource( - imageBuild.metadata.name as string, + imageBuildName, imageBuild.spec.destination, format, );libs/types/scripts/openapi-utils.js (1)
25-47: Misleading function name:copyDiractually moves files.The function copies file contents then deletes the source files (Line 43:
await fsPromises.unlink(srcPath)), making this a move operation rather than a copy. This could lead to confusion and accidental data loss if callers expect source files to remain intact.Consider renaming to
moveDiror removing the unlink call if copy semantics are intended.♻️ Suggested rename
-// Recursively copy directory -async function copyDir(src, dest) { +// Recursively move directory contents +async function moveDir(src, dest) {And update the export:
module.exports = { rimraf, - copyDir, + moveDir, fixImagebuilderCoreReferences, };libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildDetailsTab.tsx (1)
44-46: Consider simplifying the filter with a type guard.The current approach works but the type assertion can be avoided with a type predicate.
♻️ Optional refinement
- const existingImageExports = imageBuild.imageExports.filter( - (imageExport) => imageExport !== undefined, - ) as ImageExport[]; + const existingImageExports = imageBuild.imageExports.filter( + (imageExport): imageExport is ImageExport => imageExport !== undefined, + );libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/ReviewStep.tsx (1)
28-29: Consolidate imports from the same module.Two separate imports from
../../../../utils/imageBuildscan be combined.♻️ Suggested consolidation
-import { getExpectedOutputImageReference, getSourceImageReference } from '../../../../utils/imageBuilds'; -import { getExportFormatLabel } from '../../../../utils/imageBuilds'; +import { + getExpectedOutputImageReference, + getExportFormatLabel, + getSourceImageReference, +} from '../../../../utils/imageBuilds';
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/OutputImageStep.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/OutputImageStep.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/RegistrationStep.tsx
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/ReviewStep.tsx
Show resolved
Hide resolved
...nts/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx
Show resolved
Hide resolved
...nts/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx
Show resolved
Hide resolved
| const isDisabledStep = (stepId: string | undefined, validStepIds: string[]) => { | ||
| if (!stepId) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I see no need to allow stepId to be undefined
| const isDisabledStep = (stepId: string | undefined, validStepIds: string[]) => { | |
| if (!stepId) { | |
| return true; | |
| } | |
| const isDisabledStep = (stepId: string, validStepIds: string[]) => { |
There was a problem hiding this comment.
Actually, can we simplify the implementation ?
const isDisabledStep = (stepId: string, validStepIds: string[]) => {
const validIndex = validStepIds.indexOf(stepId);
return validIndex === -1 || (validIndex !== orderedIds.indexOf(stepId));
};There was a problem hiding this comment.
Yes, this is quite simpler. Using it.
There was a problem hiding this comment.
There's a problem with this.
Now in Step1 I can't move to Step2 because the "destionation" field hasn't been filled and therefore is invalid.
So I don't get the chance to fix it and I'm stuck in step 1.
The original implementation disregards errors from future steps which allows for this.
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/CreateImageBuildWizard.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/CreateImageBuildWizard.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/utils.ts
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx`:
- Line 111: The isDisabled logic is wrong because exportingFormat is typed as
ExportFormatType | undefined; change the null comparison in the isDisabled
expression so it checks for undefined (or a truthy value) instead of null. For
example, update the isDisabled assignment (the variable named isDisabled near
exportingFormat and format) to use exportingFormat !== undefined &&
exportingFormat !== format or exportingFormat ? exportingFormat !== format :
false so cards are only disabled when an export format is actually active.
In `@libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx`:
- Line 119: The message string in ImageBuildRow (the JSX element
<Content>{t('Build failed. Please retry again.')}</Content>) uses redundant
phrasing "retry again"; update the translated string to a concise form such as
"Build failed. Please retry." or "Build failed. Please try again." and update
the corresponding i18n translation key/value so the t(...) call reflects the new
text; modify the string inside the Content element in ImageBuildRow and ensure
any locale files referencing that key are updated accordingly.
In `@libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx`:
- Around line 37-41: Change the loose typing of iconMap to use ExportFormatType
as the key type: replace Record<string, React.ReactElement> with
Record<ExportFormatType, React.ReactElement> for the constant iconMap (which
maps ExportFormatType values like 'vmdk', 'qcow2', 'iso' to icons
VirtualMachineIcon, CloudSecurityIcon, ServerGroupIcon) so the compiler enforces
valid keys and prevents accidental invalid lookups.
♻️ Duplicate comments (4)
libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx (1)
100-101: IncorrectcolSpanvalue for expanded row content.The expanded row's
TdhascolSpan={7}, but the table has 9 columns (select, expand, name, base image, image output, status, export images, date, actions). This causes the expanded content to not span the full table width.🐛 Proposed fix
<Tr isExpanded={isExpanded}> - <Td colSpan={7}> + <Td colSpan={9}> <ExpandableRowContent>libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx (2)
41-45: Consider usingundefinedinstead ofnullfor error state.The state initialization uses
null, but other similar states in this component (exportingFormat,downloadingFormat) useundefined. For consistency, consider usingundefinedthroughout.♻️ Suggested change
- const [error, setError] = React.useState<{ - format: ExportFormatType; - message: string; - mode: 'export' | 'download'; - } | null>(null); + const [error, setError] = React.useState<{ + format: ExportFormatType; + message: string; + mode: 'export' | 'download'; + }>();Then use
setError(undefined)instead ofsetError(null)on lines 52 and 127.
89-99: Checkresponse.okbefore consuming the response body.The
getExportDownloadResultfunction consumesresponse.blob()for non-redirect responses before this code checksresponse.ok. For error responses (e.g., 500), the blob is read unnecessarily. Consider checkingresponse.okbefore callinggetExportDownloadResult, or restructure the utility to handle errors without consuming the body.♻️ Suggested restructure
- const downloadResult = await getExportDownloadResult(response); - if (downloadResult.type === 'redirect') { - createDownloadLink(downloadResult.url); - await showSpinnerBriefly(DOWNLOAD_REDIRECT_DELAY); - } else if (!response.ok && response.status !== 0) { - await showSpinnerBriefly(DOWNLOAD_REDIRECT_DELAY); - throw new Error(`Download failed: ${response.status} ${response.statusText}`); - } else { + // Check for errors before consuming the response body + if (!response.ok && response.status !== 0) { + await showSpinnerBriefly(DOWNLOAD_REDIRECT_DELAY); + throw new Error(`Download failed: ${response.status} ${response.statusText}`); + } + + const downloadResult = await getExportDownloadResult(response); + if (downloadResult.type === 'redirect') { + createDownloadLink(downloadResult.url); + await showSpinnerBriefly(DOWNLOAD_REDIRECT_DELAY); + } else { const defaultFilename = `image-export-${ieName}.${format.toLowerCase()}`; saveAs(downloadResult.blob, downloadResult.filename || defaultFilename); }libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
174-177: "View logs" button has no action handler.The button is rendered but does nothing when clicked. Per the PR description, this feature is not implemented due to a missing API. Consider disabling the button with a tooltip explaining the feature is coming soon, or hiding it until the API is available.
♻️ Suggested fix - disable with tooltip
<FlexItem> {exists ? ( - <Button variant="secondary">{t('View logs')}</Button> + <Button variant="secondary" isDisabled> + {t('View logs')} + </Button> ) : (Or add a Tooltip wrapper to explain why it's disabled.
🧹 Nitpick comments (5)
libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx (2)
16-24: Consider renaming prop for clarity.The prop
imageBuildis typed asImageBuildWithExports, which includes additional fields (imageExports,exportsCount) beyond a standardImageBuild. Consider renaming toimageBuildWithExportsto make it clear this isn't a plainImageBuild.♻️ Suggested rename
type ImageBuildRowProps = { - imageBuild: ImageBuildWithExports; + imageBuildWithExports: ImageBuildWithExports; rowIndex: number; onRowSelect: (imageBuild: ImageBuild) => OnSelect; isRowSelected: (imageBuild: ImageBuild) => boolean; canDelete: boolean; onDeleteClick: VoidFunction; refetch: VoidFunction; };
94-94: Unnecessary template literal.The string interpolation wrapping a number is redundant here. React handles number rendering directly.
♻️ Suggested simplification
- <Td dataLabel={t('Export images')}>{`${imageBuild.exportsCount || 0}`}</Td> + <Td dataLabel={t('Export images')}>{imageBuild.exportsCount || 0}</Td>libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/CreateImageBuildWizard.tsx (1)
143-151: Consider usingreducefor cleaner error aggregation.A past review suggested refactoring this
forEachloop to usereducefor a more functional approach. While the current implementation is correct,reducewould be slightly more idiomatic.♻️ Optional refactor using reduce
- const exportErrors: Array<{ format: ExportFormatType; error: unknown }> = []; - exportResults.forEach((result, index) => { - if (isPromiseRejected(result)) { - exportErrors.push({ - format: values.exportFormats[index], - error: result.reason, - }); - } - }); + const exportErrors = exportResults.reduce((acc, result, index) => { + if (isPromiseRejected(result)) { + acc.push({ + format: values.exportFormats[index], + error: result.reason, + }); + } + return acc; + }, [] as Array<{ format: ExportFormatType; error: unknown }>);libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx (1)
30-37: Consider defensive cleanup increateDownloadLink.If an error occurs between
appendChildandremoveChild, the link element could remain in the DOM. Usingtry/finallyensures cleanup.♻️ Suggested improvement
const createDownloadLink = (url: string) => { const link = document.createElement('a'); link.href = url; link.style.display = 'none'; document.body.appendChild(link); - link.click(); - document.body.removeChild(link); + try { + link.click(); + } finally { + document.body.removeChild(link); + } };libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
195-208: Consider if toast positioning is appropriate for inline card errors.
AlertGroup isToasttypically renders alerts in a fixed position overlay. For per-card errors displayed within the card's footer, a non-toastAlertmight provide better context by keeping the error visually attached to the affected card.♻️ Alternative: Use inline Alert
{error && ( - <AlertGroup isToast> - <Alert - variant="danger" - title={ - error.mode === 'export' ? t("We couldn't export your image") : t("We couldn't download your image") - } - actionClose={<AlertActionCloseButton onClose={onDismissError} />} - > - {t('Something went wrong on our end. Please review the error details and try again.')} - <details>{error.message}</details> - </Alert> - </AlertGroup> + <Alert + variant="danger" + isInline + title={ + error.mode === 'export' ? t("We couldn't export your image") : t("We couldn't download your image") + } + actionClose={<AlertActionCloseButton onClose={onDismissError} />} + > + {t('Something went wrong on our end. Please review the error details and try again.')} + <details>{error.message}</details> + </Alert> )}
libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildExportsGallery.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx
Outdated
Show resolved
Hide resolved
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx
Outdated
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-2611-image-building
branch
2 times, most recently
from
147b928 to
d988dce
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildDetailsPage.tsx`:
- Around line 53-64: The actions dropdown is gated by canDelete which hides the
"Duplicate/Retry" item for users who can edit but not delete; update the
conditional rendering so each DropdownItem is shown based on the correct
permission: keep the delete menu item wrapped with canDelete (and calling
setIsDeleteModalOpen), and render the "Duplicate/Retry" item when the user has
edit/create permission (e.g., a new canEdit or canCreate flag) which uses
navigate({ route: ROUTE.IMAGE_BUILD_EDIT, postfix: imageBuildId }) and respects
hasFailed for the label; ensure DetailsPageActions/DropdownList still render
when at least one action is visible.
In `@libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx`:
- Around line 183-187: The Created date currently calls
getDateDisplay(imageExport?.metadata.creationTimestamp) even when imageExport is
undefined, causing "undefined" to render; update the JSX in ImageExportCards.tsx
to conditionally render the StackItem/Content block (the Created: ... line using
ContentVariants.small) only when imageExport (or
imageExport?.metadata.creationTimestamp) is present, or provide a safe fallback
(e.g., nothing or a placeholder) before calling getDateDisplay so the helper is
never invoked with undefined.
In
`@libs/ui-components/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx`:
- Around line 76-79: The delete loop currently calls
remove(`imagebuilds/${imageBuild.metadata.name}`) without guarding metadata.name
which can send imagebuilds/undefined; update the logic in the imageBuilds.map
block (the mapping that calls remove and setProgress) to first read
imageBuild.metadata?.name into a local variable, skip/filter out entries with no
name (or return early from the async iterator) so you only call remove for valid
names, and only call setProgress for items you actually attempted to delete; use
the existing imageBuilds.map / remove / setProgress symbols to locate and change
the code.
♻️ Duplicate comments (1)
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
167-180: "View logs" button is rendered without an action handler.The button at Line 169 has no
onClickhandler and will do nothing when clicked. Per the PR description, this feature is not implemented due to a missing API. Consider disabling the button until the functionality is available, or removing it entirely for now.♻️ Suggested fix - disable until implemented
<FlexItem> {exists ? ( - <Button variant="secondary">{t('View logs')}</Button> + <Button variant="secondary" isDisabled> + {t('View logs')} + </Button> ) : (
🧹 Nitpick comments (7)
libs/ui-components/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx (2)
125-135: Use explicit boolean check to avoid rendering0.The pattern
{errors?.length && (...)}can render the literal0iferrorsis ever set to an empty array, since React renders falsy numbers. While the current code only setserrorstoundefinedor a non-empty array, this is a fragile pattern that could regress.Suggested fix
- {errors?.length && ( + {errors && errors.length > 0 && (
99-101: Consider using a plural-aware translation string.The message says "this build" (singular), but
count: imageBuildsCountis passed and the modal handles multiple builds. For proper i18n/UX, the message should reflect the count:- {t('This will remove the record of this build and its history.', { count: imageBuildsCount })} + {t('This will remove the record of {{count}} build(s) and their history.', { count: imageBuildsCount })}Or use separate singular/plural keys per your i18n setup.
libs/ui-components/src/utils/imageBuilds.ts (1)
20-29: Add a default case for exhaustive handling.The switch statement doesn't handle unknown
ExportFormatTypevalues. If a new format is added to the enum, this function will returnundefinedimplicitly, which may cause runtime issues.♻️ Suggested fix
export const getExportFormatDescription = (t: TFunction, format: ExportFormatType) => { switch (format) { case ExportFormatType.ExportFormatTypeVMDK: return t('For enterprise virtualization platforms.'); case ExportFormatType.ExportFormatTypeQCOW2: return t('For virtualized edge workloads and OpenShift Virtualization.'); case ExportFormatType.ExportFormatTypeISO: return t('For physical edge devices and bare metal.'); + default: + return t('Unknown format'); } };libs/ui-components/src/components/ImageBuilds/ImageBuildRow.tsx (1)
80-84: Use functional state update for the expand toggle.This avoids stale state if multiple toggles are queued or batched.
♻️ Suggested update
- onToggle: () => setIsExpanded(!isExpanded), + onToggle: () => setIsExpanded((prev) => !prev),libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/SourceImageStep.tsx (1)
65-67: NestedFormSectionmay cause unintended spacing/styling.There's a
FormSection(Line 65) nested inside anotherFormSection(Line 41). This could result in extra padding/margins. Consider whetherImageUrlCardshould be a direct sibling of the other form groups instead.♻️ Suggested simplification
</FormGroup> - <FormSection> - <ImageUrlCard imageReference={imageReference} /> - </FormSection> + <ImageUrlCard imageReference={imageReference} /> </FormSection>libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/CreateImageBuildWizard.tsx (1)
124-130: VariableimageBuildshadows the hook-returned value.The local
imageBuild(Line 125) shadows theimageBuildfromuseEditImageBuild()(Line 72). While not a bug since they're in different scopes, consider renaming tonewImageBuildorcreatedImageBuildfor clarity.♻️ Suggested rename
try { - const imageBuild = getImageBuildResource(values); - buildName = imageBuild.metadata.name as string; - const createdBuild = await post<ImageBuild>('imagebuilds', imageBuild); + const newImageBuild = getImageBuildResource(values); + buildName = newImageBuild.metadata.name as string; + const createdBuild = await post<ImageBuild>('imagebuilds', newImageBuild); if (createdBuild.metadata.name !== buildName) { throw new Error(t('ImageBuild was created but has a different name')); }libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildDetailsPage.tsx (1)
73-73: Logs tab placeholder acknowledged.The PR description mentions logs are not implemented due to a missing API. Consider adding a more user-friendly placeholder message instead of "TODO Logs" that explains the feature is coming soon.
Would you like me to suggest a placeholder component that displays a user-friendly message, or open an issue to track this TODO?
libs/ui-components/src/components/ImageBuilds/ImageBuildDetails/ImageBuildDetailsPage.tsx
Show resolved
Hide resolved
...nts/src/components/modals/massModals/MassDeleteImageBuildModal/MassDeleteImageBuildModal.tsx
Show resolved
Hide resolved
celdrake
force-pushed
the
EDM-2611-image-building
branch
2 times, most recently
from
2282ac5 to
f1036f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/i18n/locales/en/translation.json`:
- Around line 800-846: Standardize terminology and casing: replace inconsistent
keys/strings so "repository" vs "registry" and "ImageBuild" vs "Image build" are
consistent across the file—use "repository" everywhere (change error keys
"Source registry is required" and "Target registry is required" to "Source
repository is required" and "Target repository is required"), and rename the
string "ImageBuild was created but has a different name" to "Image build was
created but has a different name" to match other keys like "Failed to create
image build" and "Image build created, but some exports failed"; update any
corresponding usages of these message keys in code to the new key names if
necessary (look for references to "Source repository", "Target repository",
"ImageBuild was created but has a different name", "Failed to create image
build", and "Image build created, but some exports failed").
♻️ Duplicate comments (1)
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
167-179: "View logs" button lacks an action handler.The button renders but performs no action. Since the PR notes this feature isn't implemented due to a missing API, consider either disabling the button or adding a tooltip explaining the limitation until the API is available.
🧹 Nitpick comments (2)
libs/ui-components/src/components/ImageBuilds/ImageExportCards.tsx (1)
188-200: Consider removingisToastfor inline error display.
AlertGroupwithisToastrenders alerts as floating notifications at a fixed viewport position, which conflicts with the inline CardFooter placement. If multiple cards have errors, their toasts would overlap at the same position.For inline error display within the card, remove
isToastand use a plainAlert:♻️ Suggested change for inline error display
{error && ( - <AlertGroup isToast> - <Alert - variant="danger" - title={ - error.mode === 'export' ? t("We couldn't export your image") : t("We couldn't download your image") - } - actionClose={<AlertActionCloseButton onClose={onDismissError} />} - > - {t('Something went wrong on our end. Please review the error details and try again.')} - <details>{error.message}</details> - </Alert> - </AlertGroup> + <Alert + variant="danger" + title={ + error.mode === 'export' ? t("We couldn't export your image") : t("We couldn't download your image") + } + actionClose={<AlertActionCloseButton onClose={onDismissError} />} + > + {t('Something went wrong on our end. Please review the error details and try again.')} + <details>{error.message}</details> + </Alert> )}libs/ui-components/src/components/ImageBuilds/CreateImageBuildWizard/steps/SourceImageStep.tsx (1)
65-67: Consider removing the nestedFormSectionfor consistency.The inner
FormSectionwrapping onlyImageUrlCardcreates unnecessary DOM nesting. InOutputImageStep.tsx(line 97),ImageUrlCardis placed directly within the parentFormSectionwithout additional wrapping.♻️ Suggested change
</FormGroup> - <FormSection> - <ImageUrlCard imageReference={imageReference} /> - </FormSection> + <ImageUrlCard imageReference={imageReference} /> </FormSection>
celdrake
force-pushed
the
EDM-2611-image-building
branch
from
f1036f7 to
1dbd56a
Compare
2 participants
Not implemented yet due to missing API
Summary by CodeRabbit
New Features
Configuration
Documentation
UX
✏️ Tip: You can customize this high-level summary in your review settings.