-
Notifications
You must be signed in to change notification settings - Fork 68
Wizard: fix repeatable build behavior in pkg and repo steps #3731
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wizard: fix repeatable build behavior in pkg and repo steps #3731
Conversation
eac84a4 to
f95ef97
Compare
|
/ok-to-test |
|
Quick heads up - the manual API changes check failure will most probably get resolved after rebase. |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #3731 +/- ##
==========================================
+ Coverage 79.85% 79.97% +0.11%
==========================================
Files 222 222
Lines 25762 25824 +62
Branches 2500 2529 +29
==========================================
+ Hits 20573 20652 +79
+ Misses 5160 5143 -17
Partials 29 29
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
f95ef97 to
2aa6466
Compare
2aa6466 to
2bfe8f0
Compare
|
/ok-to-test |
2bfe8f0 to
fb86b31
Compare
|
/ok-to-test |
regexowl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this works great! ❤️
Two questions - do the tables have to be this different (between with and without repeatable build) or could we just:
- render plain/link packages count based on status of repeatable build
- render snapshot date column based on the same
- as for the repository link under the repository name that gets rendered with no repeatable build, that one could be completely removed. We're in the middle of UI revamp and it's not in final mocks so there's no need to conditionally switch between hiding/rendering it
Second question which is more or less based on what the first answer is - there's a bunch of duplicate code now. Could we move some of it to reusable components and use those so we don't have to update code in two places when we need to for example rename an alert or table heading?
fb86b31 to
a0bc5fe
Compare
@regexowl thanks for taking a look at this! 🫶 also made small adjustments to the loading states so it's not reloading the whole step so much and looks a bit nicer (there was also an issue with the templates page flickering the amount of repos (showing incorrect ones), which shouldn't be happening anymore) 😅 |
|
/ok-to-test |
a0bc5fe to
e549ec2
Compare
|
/ok-to-test |
e549ec2 to
732fcff
Compare
|
/ok-to-test |
📝 WalkthroughWalkthroughThe pull request adds snapshot date support across the Create Image Wizard. Packages now read Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
1286-1302: Add optional chaining to prevent potential crash.Line 1286 correctly uses optional chaining (
grp.package_list?.length), but line 1292 maps overgrp.package_listwithout optional chaining. Ifpackage_listis undefined (as the PR notes can happen with empty package groups in Red Hat repos), this would throw a runtime error.Apply this diff to add defensive optional chaining:
- {grp.package_list.map((pkg) => ( + {grp.package_list?.map((pkg) => ( <Tr key={`details-${pkg}`}>Alternatively, if
package_listis guaranteed to be an array (possibly empty) when it passes the length check, add a comment explaining this invariant.
🧹 Nitpick comments (1)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
304-306: Simplify the date conversion chain.The conversion
snapshotDate → convertStringToDate() → new Date() → toISOString()appears redundant. TheconvertStringToDateutility returns a timestamp (number), which is then wrapped innew Date()and converted back to ISO string.Consider simplifying to:
date: snapshotDate ? new Date(snapshotDate).toISOString() : undefined,Unless
convertStringToDateperforms special parsing logic that's required here, this simpler approach would be more readable and efficient.Can you confirm whether the
convertStringToDateutility is necessary for parsing thesnapshotDateformat, or if it can be simplified as suggested?Also applies to: 321-323, 331-333, 373-375, 386-388, 396-398
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx(10 hunks)src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx(10 hunks)src/Utilities/time.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx (7)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectSnapshotDate(465-467)src/store/contentSourcesApi.ts (1)
useListSnapshotsByDateMutation(8-10)src/Utilities/time.ts (2)
convertStringToDate(59-68)timestampToDisplayStringDetailed(36-57)src/Components/CreateImageWizard/steps/Repositories/components/Error.tsx (1)
Error(5-11)src/Components/CreateImageWizard/steps/Repositories/components/Loading.tsx (1)
Loading(5-15)src/constants.ts (1)
CONTENT_URL(13-13)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (3)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectSnapshotDate(465-467)src/Utilities/time.ts (1)
convertStringToDate(59-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: Check for Blocked Packages
- GitHub Check: playwright-tests
- GitHub Check: Manual API Changes Check
- GitHub Check: Build Check
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (9)
src/Utilities/time.ts (1)
36-36: LGTM! Timezone parameter addition is well-implemented.The optional
tzparameter is backward compatible and properly integrated. Whentzis undefined, theIntl.DateTimeFormatwill use the default behavior (typically the runtime's timezone), which is correct.Also applies to: 52-52
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (3)
362-362: Good modernization: substr → substring.Replacing the deprecated
substrwithsubstringis a best practice update.Also applies to: 382-382, 394-394
352-352: LGTM! Dependency arrays properly updated.The
snapshotDateis correctly added to the dependency arrays of theuseEffecthooks that trigger package and group searches, ensuring searches re-run when the snapshot date changes.Also applies to: 415-415
96-96: selectSnapshotDate is properly exported and correctly imported.The selector is correctly exported from
src/store/wizardSlice.ts(line 465) and properly imported in Packages.tsx at line 96. The usage at line 149 withuseAppSelectoris correct.Note: The reference to line 104 appears to be stale—there is no
selectSnapshotDatereference at that location; only lines 96 and 149 contain it.src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx (5)
16-16: LGTM! Imports properly added for snapshot functionality.All necessary imports for snapshot date handling are correctly added, including the mutation hook, selectors, utilities, and UI components.
Also applies to: 41-41, 51-51, 65-65, 71-74, 88-88
524-558: Verify the snapshot fetching logic and dependencies.The useEffect correctly skips fetching when snapshots aren't needed (no date, using latest content, or template selected). The dependency array is comprehensive.
However, ensure the
contentList.lengthcheck doesn't cause issues on the initial render when the list might be empty before repositories load.Please verify that the snapshot fetch doesn't get skipped incorrectly when repositories are still loading initially.
781-782: Verify EPEL warning is intentionally hidden in snapshot mode.The EPEL warning (
CustomEpelWarning) is now only rendered when!snapshotDate. This means users won't see the warning when viewing repositories with a snapshot date selected.If this is intentional (perhaps because the warning is only relevant for latest content), consider adding a comment explaining why. If the warning should appear in both modes, remove the
!snapshotDatecondition.Confirm whether the EPEL warning should be shown in snapshot mode or if hiding it is the intended behavior.
721-839: LGTM! Conditional table rendering is well-implemented.The snapshot-driven display mode is cleanly implemented with:
- Proper conditional column headers (Architecture/Version/Status vs. Snapshot date)
- Correct use of the new time formatting utility with UTC timezone
- External links to snapshot details in the Content UI
- Loading spinners during snapshot data fetch
- Fallback to '-' when snapshot data is unavailable
The implementation maintains table structure consistency and handles edge cases appropriately.
779-783: Community repository URLs are correctly hidden in snapshot mode and shown in non-snapshot mode.The simplification was intentional. The commit message indicates this change "hides information about repositories that might be incorrect at the chosen date". Community repositories follow the same rendering pattern as all other repository types:
- When
snapshotDateis set: detailed columns (including Status with URLs) are hidden for all repos to avoid showing potentially stale information- When
snapshotDateis not set (!snapshotDate): the Status column displays URLs for all repos, including Community repos, via theRepositoriesStatuscomponentThe
CommunityRepositoryLabelcomponent serves as a visual indicator and tooltip, while URL display is handled consistently for all repository types through the conditional Status column. This design is correct and no changes are needed.
This fixes a bug that occurs in the packages search step, when any of the repos have a package group with an empty package list (this can be the case for Red Hat AppStream conflict groups).
This adds a different view for the 'Repositories' step that reflects if repeatable build with a set snapshot date was chosen. It hides information about repositories that might be incorrect at the chosen date and present a snapshot date and displays correct package count with a link to the snapshot detail.
This fixes an issue with the 'Packages' step where you could pick additional packages that might not be available in the included repositories at the chosen snapshot date, or ones that were removed. This would be very complicated to do solely in this frontend so the names search APIs were extended to support a date argument and search the correct snapshots for the content when that's provided.
732fcff to
980e6a5
Compare
|
/ok-to-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (1)
268-353: Consider addingreposInTemplateto the dependency array.The effect uses
reposInTemplateat lines 297-301 but doesn't include it in the dependency array (lines 338-353). WhileisLoadingReposInTemplateis included and may trigger re-runs indirectly, best practice is to include all values referenced in the effect body.Apply this diff to add the missing dependency:
}, [ customRepositories, searchCustomRpms, searchDistroRpms, debouncedSearchTerm, activeTabKey, searchRecommendedRpms, epelRepoUrlByDistribution, isSuccessDistroRepositories, distroRepositories, arch, template, distribution, debouncedSearchTermIsGroup, snapshotDate, + reposInTemplate, ]);src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx (1)
756-760: Conditional rendering correctly implements snapshot mode, but improve type safety at line 827.The snapshot-aware rendering is well-implemented: derives per-repo snapshot data, shows loading spinners during fetch, displays snapshot date and package counts, and links to snapshot details. However, line 827 accesses
snapshot.match.uuidwithout optional chaining inside a condition that uses optional chaining (line 819).While logically safe (the condition ensures it exists), TypeScript may not narrow the type properly. For type safety and consistency, apply this diff:
href={`${CONTENT_URL}/${uuid}/snapshots/${snapshot.match.uuid}`} + href={`${CONTENT_URL}/${uuid}/snapshots/${snapshot?.match?.uuid}`}Also applies to: 779-783, 785-839
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx(10 hunks)src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx(10 hunks)src/Utilities/time.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utilities/time.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (3)
src/store/hooks.ts (1)
useAppSelector(7-7)src/store/wizardSlice.ts (1)
selectSnapshotDate(465-467)src/Utilities/time.ts (1)
convertStringToDate(59-68)
src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx (6)
src/store/wizardSlice.ts (1)
selectSnapshotDate(465-467)src/store/contentSourcesApi.ts (1)
useListSnapshotsByDateMutation(8-10)src/Utilities/time.ts (2)
convertStringToDate(59-68)timestampToDisplayStringDetailed(36-57)src/Components/CreateImageWizard/steps/Repositories/components/Error.tsx (1)
Error(5-11)src/Components/CreateImageWizard/steps/Repositories/components/Loading.tsx (1)
Loading(5-15)src/constants.ts (1)
CONTENT_URL(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: Build Check
- GitHub Check: Manual API Changes Check
- GitHub Check: Circular Dependencies Check
- GitHub Check: ESLint
- GitHub Check: Service Tests
- GitHub Check: Cockpit Tests
- GitHub Check: Check for Blocked Packages
- GitHub Check: testing-farm:fedora-41-x86_64:self
- GitHub Check: testing-farm:fedora-43-x86_64:self
- GitHub Check: testing-farm:fedora-42-x86_64:self
- GitHub Check: testing-farm:centos-stream-10-x86_64:self
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-aarch64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-10-aarch64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Sourcery review
- GitHub Check: Red Hat Konflux / image-builder-frontend-on-pull-request
🔇 Additional comments (9)
src/Components/CreateImageWizard/steps/Packages/Packages.tsx (5)
96-96: LGTM! Snapshot date integration properly initialized.The imports of
selectSnapshotDateandconvertStringToDate, along with readingsnapshotDatefrom the store, correctly set up the component to filter packages by snapshot date.Also applies to: 104-104, 149-149
304-306: LGTM! Date parameter consistently integrated across RPM searches.The
datefield is correctly added to distro, custom, and recommended RPM search API calls with consistent conversion logic. ThesnapshotDatedependency ensures searches re-execute when the date changes.Also applies to: 321-323, 331-333, 352-352
362-362: LGTM! Group searches updated with date parameters and modernized string method.The
dateparameter is consistently added to all group search API calls, andsubstring(1)correctly replaces the deprecatedsubstr(1)method for trimming the@prefix from group searches.Also applies to: 373-375, 382-382, 386-388, 394-394, 396-398, 415-415
1281-1281: LGTM! Optional chaining prevents crash on empty package lists.The optional chaining on
grp.package_list?.lengthsafely handles package groups with undefined or empty package lists, fixing the crash issue mentioned in the PR objectives.Also applies to: 1286-1286
1464-1464: LGTM! Dependency array correctly includes transformed groups.Adding
transformedGroupsto thebodyContentmemoization dependencies ensures the table body re-renders when group data changes.src/Components/CreateImageWizard/steps/Repositories/Repositories.tsx (4)
16-16: LGTM! Snapshot infrastructure properly imported and initialized.The necessary imports for snapshot functionality (mutation hook, selectors, utilities, UI components) and reading
snapshotDatefrom the store correctly set up the component for snapshot-aware repository display.Also applies to: 41-41, 51-51, 65-65, 71-74, 88-88
510-510: LGTM! Snapshot fetching logic correctly implemented.The
useEffectappropriately fetches snapshots when a snapshot date is selected (and not using latest content or templates). The date conversion is consistent with Packages.tsx, and the dependency array is complete.Also applies to: 524-558
595-611: LGTM! Error and loading states correctly handle snapshot data.The error condition appropriately includes
isSnapshotsError, and the loading logic for snapshots (isSnapshotsLoading && isSnapshotsUninitilized) intentionally shows full-page loading only on initial fetch while subsequent fetches display spinners in table cells (lines 814, 835).
721-733: LGTM! Table headers conditionally render for snapshot mode.The conditional headers correctly display snapshot-specific columns (Snapshot date and Packages) when a snapshot date is selected, hiding metadata that may not reflect the historical snapshot state.
regexowl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks a lot! 🚀
Summary
This PR includes a few changes and fixes to correct the behavior of the wizard when a repeatable build with a snapshot date is chosen.
This would be very complicated to do solely in this frontend, so the names search APIs were extended to support a date argument and search the correct snapshots for the right content when that's provided. [PR]
Also fixes a bug found while testing, where the package search step would outright crash when a package group with an empty package list existed in the searched repos. (This can happen in Red Hat repos in the Conflict package groups)
Issue Link: [HMS-8645]
Testing Steps
v1URL and then update it to thev2URL.2.1 If you select a date few days in the past, the picked up snapshot will be the V1 and if you select use latest, or a date after the repo was created, it should be the second one.