Conversation
Reviewer's GuideThis pull request implements a new "Bulk Repository Sets Wizard" allowing users to apply content overrides (enable, disable, reset) to repository sets across multiple hosts. The wizard is launched from the Hosts Actions Bar and uses a multi-step interface built with PatternFly components. State is managed via React Context. The first step involves selecting repository sets and actions using a File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @parthaa - I've reviewed your changes - here's some feedback:
- Update the PR title and description to accurately reflect the introduced feature and all related changes.
- Consider moving the button reordering in the existing Errata and Packages wizard footers to a separate PR to maintain focus.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| const handleFinishButtonClick = () => { | ||
| setFinishButtonLoading(true); | ||
| saveContentOverrides(); | ||
| closeModal(); |
There was a problem hiding this comment.
question (bug_risk): Revisit the closeModal invocation in handleFinishButtonClick.
saveContentOverrides already triggers closeModal on success. Please remove the redundant closeModal call or confirm it’s safe to call twice.
| const [, setCurrentStep] = useState(); | ||
| const onStepChange = (_event, newStep) => setCurrentStep((oldStep) => { | ||
| setShouldValidateStep1(true); | ||
| if (oldStep === 'brsw-step-2') setShouldValidateStep2(true); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (oldStep === 'brsw-step-2') setShouldValidateStep2(true); | |
| if (oldStep === 'brsw-step-2') { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
| const apiOptions = { key: 'BULK_HOST_REPO_SETS' }; | ||
|
|
||
| const finishButtonText = __('Set content overrides'); | ||
| const replacementResponse = !modalOpen ? { response: {} } : false; |
There was a problem hiding this comment.
suggestion (code-quality): Invert ternary operator to remove negation (invert-ternary)
| const replacementResponse = !modalOpen ? { response: {} } : false; | |
| const replacementResponse = modalOpen ? false : { response: {} }; |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By inverting the ternary condition and swapping the
expressions we can simplify the code.
| ...bulkParams, | ||
| successToast: () => __('Content overrides updating.'), | ||
| handleSuccess: (response) => { | ||
| if (handleSuccess) handleSuccess(response); |
There was a problem hiding this comment.
suggestion (code-quality): Use block braces for ifs, whiles, etc. (use-braces)
| if (handleSuccess) handleSuccess(response); | |
| if (handleSuccess) { |
Explanation
It is recommended to always use braces and create explicit statement blocks.Using the allowed syntax to just write a single statement can lead to very confusing
situations, especially where subsequently a developer might add another statement
while forgetting to add the braces (meaning that this wouldn't be included in the condition).
Refs #38353 - add toggle dropdown Refs #38353 - add host review and step validation Refs #38353 - change to table component with children Refs #38353 - add expandable table rows and semi-broken pagination Refs #38353 - fix pagination Refs #38353 - Add review footer Refs #38353 - Hook it up to the API Refs #38353 - address comments & fix export Refs #38353 - Move primary buttons to be consistent Refs #38353 - address comments
WalkthroughA new bulk repository sets management feature is introduced, including a multi-step wizard modal for bulk editing repository set statuses on hosts. This involves new React components, context, Redux actions, helpers, and UI integrations. Additionally, button ordering in existing bulk wizards is updated, and modal registration and menu options are extended to support the new functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ActionsBar
participant ModalSystem
participant BulkRepoSetsWizard
participant API
User->>ActionsBar: Clicks "Repository sets" menu item
ActionsBar->>ModalSystem: Open 'bulk-repo-sets-wizard' modal
ModalSystem->>BulkRepoSetsWizard: Render wizard
BulkRepoSetsWizard->>API: Fetch repository sets data
User->>BulkRepoSetsWizard: Selects repos, sets overrides, proceeds through steps
BulkRepoSetsWizard->>API: Fetches/updates selection as needed
User->>BulkRepoSetsWizard: Clicks "Finish"
BulkRepoSetsWizard->>API: PUT /hosts/bulk/content_overrides with overrides
API-->>BulkRepoSetsWizard: Responds with task info
BulkRepoSetsWizard->>ModalSystem: Close modal
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
webpack/components/extensions/Hosts/ActionsBar/index.jsError: Cannot read config file: /webpack/.eslintrc.js webpack/components/extensions/Hosts/BulkActions/BulkErrataWizard/04_ReviewFooter.jsError: Cannot read config file: /webpack/.eslintrc.js webpack/components/extensions/Hosts/BulkActions/BulkPackagesWizard/04_ReviewFooter.jsError: Cannot read config file: /webpack/.eslintrc.js
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/actions.js (1)
16-16: Use block braces for single-line if statements.It's recommended to always use braces for if statements, even for single lines, to prevent potential bugs when adding more statements later.
- if (handleSuccess) handleSuccess(response); + if (handleSuccess) { + handleSuccess(response); + }webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/03_ReviewFooter.js (1)
41-49:⚠️ Potential issueRedundant & premature
closeModal()call – re-consider flow
saveContentOverrides()already passescloseModalas both the success and failure callback when dispatchingbulkUpdateHostContentOverrides.
CallingcloseModal()again immediately after dispatch:const handleFinishButtonClick = () => { setFinishButtonLoading(true); saveContentOverrides(); closeModal(); // ← duplicates callbacks & closes modal before API returns };results in the modal being closed three times and, more importantly, unmounts the wizard before the async Redux action completes, so the user never sees success/error toasts and the component tree that owns
setFinishButtonLoadingis gone.Suggested fix: let the action creator be responsible for closing the modal once.
const handleFinishButtonClick = () => { setFinishButtonLoading(true); - saveContentOverrides(); - closeModal(); + saveContentOverrides(); };
🧹 Nitpick comments (4)
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/helpers.js (1)
1-26: Replace magic numbers with named constantsThe function uses magic numbers (0, 1, 2, 3) to represent different override states. Using named constants would improve code readability and maintainability.
+// Constants for repository override states +export const OVERRIDE_STATES = { + NO_CHANGE: 0, + ENABLE: 1, + DISABLE: 2, + RESET: 3, +}; + export const pendingOverrideToApiParamItem = ({ repoLabel, value }) => { switch (Number(value)) { - case 0: // No change + case OVERRIDE_STATES.NO_CHANGE: return null; - case 1: // Override to enabled + case OVERRIDE_STATES.ENABLE: return { content_label: repoLabel, name: 'enabled', value: true, }; - case 2: // Override to disabled + case OVERRIDE_STATES.DISABLE: return { content_label: repoLabel, name: 'enabled', value: false, }; - case 3: // Reset to default + case OVERRIDE_STATES.RESET: return { content_label: repoLabel, name: 'enabled', remove: true, }; default: return null; } };webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/actions.js (1)
6-6: Consider prefixing API constant with the module name.To avoid potential naming conflicts, consider prefixing the API constant with the module name or feature area.
-const BULK_HOST_CONTENT_OVERRIDES_KEY = 'BULK_HOST_CONTENT_OVERRIDES'; +const BULK_REPO_SETS_CONTENT_OVERRIDES_KEY = 'BULK_REPO_SETS_CONTENT_OVERRIDES';Then update the reference on line 11 accordingly.
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/BulkRepositorySetsWizard.js (1)
41-44: Ternary could be simplified for readability
replacementResponse = !modalOpen ? { … } : false;contains a negation.
Flip the condition to avoid the mental “double negative”.-const replacementResponse = !modalOpen ? { response: {} } : false; +const replacementResponse = modalOpen ? false : { response: {} };webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/01_BulkRepositorySetsTable.js (1)
177-191:handleActionTogglenever updatesactionDropdownValueWhen users pick a value from the split-button action, the main button keeps showing the previous label.
CallsetActionDropdownValue(value)so the label reflects the last applied action.const handleActionToggle = (value) => { const result = {}; selectedResults.forEach((repo) => { result[repo.label] = value; }); setPendingOverrides({ ...pendingOverrides, ...result }); setShouldValidateStep1(true); + setActionDropdownValue(value); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
webpack/components/extensions/Hosts/ActionsBar/index.js(2 hunks)webpack/components/extensions/Hosts/BulkActions/BulkErrataWizard/04_ReviewFooter.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkPackagesWizard/04_ReviewFooter.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/01_BulkRepositorySetsTable.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/03_Review.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/03_ReviewFooter.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/BulkRepositorySetsWizard.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/actions.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/helpers.js(1 hunks)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/index.js(1 hunks)webpack/global_index.js(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
webpack/components/extensions/Hosts/BulkActions/BulkErrataWizard/04_ReviewFooter.js (1)
webpack/components/extensions/Hosts/BulkActions/BulkPackagesWizard/04_ReviewFooter.js (1)
finishButton(103-130)
webpack/global_index.js (2)
webpack/components/extensions/Hosts/BulkActions/BulkErrataWizard/index.js (1)
BulkErrataWizardModal(6-22)webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/index.js (1)
BulkRepositorySetsWizardModal(6-21)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Ruby / Setup matrix
- GitHub Check: react-tests / Foreman develop Ruby 2.7 and Node 18
- GitHub Check: Angular bastion_katello - NodeJS 18
🔇 Additional comments (15)
webpack/components/extensions/Hosts/BulkActions/BulkPackagesWizard/04_ReviewFooter.js (1)
137-137: Button order standardized for consistency.The repositioning of the finish button between the Back and Cancel buttons improves UI consistency across all wizard footers in the application.
webpack/components/extensions/Hosts/BulkActions/BulkErrataWizard/04_ReviewFooter.js (1)
95-95: Button order standardized for consistency.The repositioning of the finish button between the Back and Cancel buttons improves UI consistency across all wizard footers in the application.
webpack/global_index.js (2)
41-41: New import for the BulkRepositorySetsWizardModal.The import is correctly placed with the other bulk wizard modal imports.
100-101: Proper modal registration with priority ordering.The changes:
- Update the priority of BulkErrataWizardModal from 200 to 300
- Add the new BulkRepositorySetsWizardModal with priority 400
This ensures consistent ordering in the UI and properly integrates the new bulk repository sets wizard.
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/index.js (1)
1-24: Well-structured modal component for the new bulk repository sets wizard.The implementation follows the established pattern for modal components in the application:
- Uses the
useForemanModalhook for modal state management- Sets appropriate modal properties including accessibility attributes
- Provides proper OUIA ID for testing
- Renders the wizard component within the modal
webpack/components/extensions/Hosts/ActionsBar/index.js (3)
43-43: LGTM! New repository sets wizard modal registration.The modal registration for the new bulk repository sets wizard is correctly added to the list of managed modals.
51-51: LGTM! Modal hook implementation for repository sets wizard.The hook for controlling the repository sets wizard modal state is properly implemented following the existing pattern.
89-97: LGTM! Well-implemented Repository sets menu item.The Repository sets menu item is properly implemented with appropriate disabled state handling and organization context validation, matching the pattern of other menu items.
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/03_Review.js (3)
1-17: LGTM! Clean imports and context setup.The imports and context setup are well organized, properly importing necessary dependencies and the wizard context.
18-27: LGTM! Well-implemented state management and data transformation.The component efficiently retrieves and filters pending overrides from context, mapping numeric values to descriptive texts.
28-76: LGTM! Clean UI implementation with appropriate OUIA IDs.The UI implementation is well-structured using Patternfly components with appropriate OUIA IDs for accessibility and testing.
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/actions.js (1)
8-22: LGTM! Well-implemented API action.The API call is correctly implemented using the put function, with proper success and error handling.
webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/03_ReviewFooter.js (1)
45-47:finishButtonLoadingis never reset on failureWhen the PUT request fails, the failure callback only closes the modal; if you decide to keep the wizard open (see previous comment), remember to call
setFinishButtonLoading(false)in the error path so the button becomes usable again.webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/BulkRepositorySetsWizard.js (1)
84-92: Validation helper silently acceptsNaN
selectionIsValid = count > 0 || isNaN(count)means an undefined/NaN count is treated as valid, potentially allowing users to proceed without any selection.
Consider making the intent explicit:-const selectionIsValid = count => count > 0; +const selectionIsValid = count => Number.isFinite(count) && count > 0;webpack/components/extensions/Hosts/BulkActions/BulkRepositorySetsWizard/01_BulkRepositorySetsTable.js (1)
123-126:useSetexpansion key uniqueness
repoIsExpandedrelies onlabel. If two repositories share a label (edge-case but possible across different products), expansion states will collide. Consider using a compound key (productId-label) instead.
| const saveContentOverrides = () => { | ||
| const requestBody = { | ||
| included: { | ||
| search: hostsBulkSelect.fetchBulkParams(), | ||
| }, | ||
| content_overrides: apiParams, | ||
| }; | ||
| dispatch(bulkUpdateHostContentOverrides( | ||
| requestBody, hostsBulkSelect.fetchBulkParams(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Duplicated fetchBulkParams() argument is unnecessary
bulkUpdateHostContentOverrides receives requestBody and the exact same fetchBulkParams() value again.
If the action creator doesn’t require it twice, remove the second argument to avoid confusing API signatures.
- dispatch(bulkUpdateHostContentOverrides(
- requestBody, hostsBulkSelect.fetchBulkParams(),
- closeModal, closeModal,
- ));
+ dispatch(bulkUpdateHostContentOverrides(
+ requestBody,
+ closeModal, closeModal,
+ ));📝 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 saveContentOverrides = () => { | |
| const requestBody = { | |
| included: { | |
| search: hostsBulkSelect.fetchBulkParams(), | |
| }, | |
| content_overrides: apiParams, | |
| }; | |
| dispatch(bulkUpdateHostContentOverrides( | |
| requestBody, hostsBulkSelect.fetchBulkParams(), | |
| const saveContentOverrides = () => { | |
| const requestBody = { | |
| included: { | |
| search: hostsBulkSelect.fetchBulkParams(), | |
| }, | |
| content_overrides: apiParams, | |
| }; | |
| dispatch(bulkUpdateHostContentOverrides( | |
| requestBody, | |
| closeModal, closeModal, | |
| )); | |
| }; |
| const DEFAULT_PER_PAGE = 5; | ||
| export const BulkRepositorySetsWizardContext = createContext({}); | ||
| export const REPO_SETS_URL = katelloApi.getApiUrl(`/repository_sets?per_page=${DEFAULT_PER_PAGE}&include_permissions=true&enabled=true&with_custom=true&organization_id=1`); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded organization_id=1 breaks multi-org installations
Embedding the org id directly in REPO_SETS_URL makes the wizard unusable for any organization except id 1. Retrieve the current organization from Redux/Context (e.g. window.ORG.id) or accept it as a prop.
-export const REPO_SETS_URL = katelloApi.getApiUrl(`/repository_sets?per_page=${DEFAULT_PER_PAGE}&include_permissions=true&enabled=true&with_custom=true&organization_id=1`);
+export const buildRepoSetsUrl = orgId =>
+ katelloApi.getApiUrl(
+ `/repository_sets?per_page=${DEFAULT_PER_PAGE}&include_permissions=true&enabled=true&with_custom=true&organization_id=${orgId}`,
+ );📝 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 DEFAULT_PER_PAGE = 5; | |
| export const BulkRepositorySetsWizardContext = createContext({}); | |
| export const REPO_SETS_URL = katelloApi.getApiUrl(`/repository_sets?per_page=${DEFAULT_PER_PAGE}&include_permissions=true&enabled=true&with_custom=true&organization_id=1`); | |
| const DEFAULT_PER_PAGE = 5; | |
| export const BulkRepositorySetsWizardContext = createContext({}); | |
| export const buildRepoSetsUrl = orgId => | |
| katelloApi.getApiUrl( | |
| `/repository_sets?per_page=${DEFAULT_PER_PAGE}&include_permissions=true&enabled=true&with_custom=true&organization_id=${orgId}`, | |
| ); |
| onOpenChange={openVal => setIsOpen(!openVal)} | ||
| toggle={toggleRef => ( | ||
| <MenuToggle ref={toggleRef} onClick={onToggleClick} isExpanded={isOpen}> | ||
| {currentLabel} |
There was a problem hiding this comment.
onOpenChange inversion causes dropdown state desync
onOpenChange already provides the new open state (isOpen).
Negating it stores the opposite value, so the UI occasionally shows the wrong state.
- onOpenChange={openVal => setIsOpen(!openVal)}
+ onOpenChange={isOpen => setIsOpen(isOpen)}Apply the same fix to setActionToggleOpen(!val) in the action dropdown.
📝 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.
| onOpenChange={openVal => setIsOpen(!openVal)} | |
| toggle={toggleRef => ( | |
| <MenuToggle ref={toggleRef} onClick={onToggleClick} isExpanded={isOpen}> | |
| {currentLabel} | |
| onOpenChange={isOpen => setIsOpen(isOpen)} | |
| toggle={toggleRef => ( | |
| <MenuToggle ref={toggleRef} onClick={onToggleClick} isExpanded={isOpen}> | |
| {currentLabel} |
2 participants
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Summary by Sourcery
Add a new wizard for bulk managing repository sets content overrides for hosts
New Features:
Enhancements:
Summary by CodeRabbit
New Features
Improvements