Fixes #39444 - Migrate Subscriptions from pf3 to pf5#11788
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:
📝 WalkthroughWalkthroughThis PR updates Subscriptions UI components, dialogs, and related Redux wiring to PatternFly React Core v5, refactors several class components to hooks-based functional components, and changes subscription table column rendering, preferences loading, and tests to match the new component structure. ChangesPatternFly v5 Migration and Subscriptions Refactors
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webpack/scenes/Subscriptions/Details/SubscriptionDetails.js`:
- Around line 41-43: The toast notification call using
window.tfm.toastNotifications.notify is currently being executed during render,
which causes it to fire on every re-render when subscriptionDetails.error
exists, resulting in duplicate alerts. Move the if check for
subscriptionDetails.error and the window.tfm.toastNotifications.notify call out
of the render logic and into a useEffect hook with subscriptionDetails.error as
a dependency in the dependency array. This ensures the notification only
triggers when the error value actually changes rather than on every render
cycle.
In `@webpack/scenes/Subscriptions/SubscriptionsPage.js`:
- Around line 63-75: The loadData function is being called from both an
initial-mount effect and an organization-change effect, causing duplicate
dispatches of pollTasks, loadSubscriptions, and loadTables when an organization
is already loaded at mount. Refactor the effect structure by either combining
both effects into a single effect that handles both initialization and
organization changes, or add a condition to skip calling loadData in the
initial-mount effect if an organization is already present in the component
state or props. Ensure loadData is only invoked once per relevant trigger rather
than on both mount and organization change.
In
`@webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js`:
- Around line 207-213: The componentRef object passed to the columns function is
missing the saveUpstreamSubscriptions method, which is needed by the quantity
cell's onKeyDown handler when Enter is pressed. Add saveUpstreamSubscriptions to
the componentRef object to expose the method required by the table schema.
Additionally, the columns function is being called with selectionController as a
second argument but the function signature only accepts one parameter
(controller), causing selectionController to be silently ignored. Either update
the columns function signature to accept selectionController as a second
parameter, or include selectionController within the componentRef object so it
can be accessed from the columns function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 962c1fac-2153-4058-b3fa-e88b01434a13
📥 Commits
Reviewing files that changed from the base of the PR and between 49b74d3 and bfd5755b8bfda6a87988eafe284972c7e3f840fd.
📒 Files selected for processing (19)
webpack/components/OptionTooltip/OptionTooltip.scsswebpack/components/OptionTooltip/__tests__/OptionTooltip.test.jswebpack/components/OptionTooltip/index.jswebpack/components/TooltipButton/TooltipButton.jswebpack/redux/reducers/index.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.jswebpack/scenes/Subscriptions/Details/SubscriptionDetails.jswebpack/scenes/Subscriptions/Details/__tests__/SubscriptionDetailProducts.test.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.scsswebpack/scenes/Subscriptions/SubscriptionsPage.jswebpack/scenes/Subscriptions/SubscriptionsPage.scsswebpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.jswebpack/scenes/Subscriptions/UpstreamSubscriptions/__tests__/UpstreamSubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js (1)
15-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLocalization regression: validation messages are no longer translated.
The migrated
quantityValidationreturns hardcoded English literals where the prior implementation used__(). These strings surface to users (rendered viaHelpBlockin the table schema), so non-English locales will now always see English. Re-wrapping with__()restores translation; since the test mock for__returns the key unchanged, the existing assertions inUpstreamSubscriptionsPage.test.jscontinue to pass.🌐 Proposed fix
- const aboveZeroMsg = [false, 'Please enter a positive number above zero']; + const aboveZeroMsg = [false, __('Please enter a positive number above zero')]; - if (parsedQuantity.toString().length > 10) return [false, 'Please limit number to 10 digits']; - if (!pool.available) return [false, 'No pools available']; + if (parsedQuantity.toString().length > 10) return [false, __('Please limit number to 10 digits')]; + if (!pool.available) return [false, __('No pools available')]; // handling unlimited subscriptions, they show as -1 if (pool.available === -1) return parsedQuantity ? [true, ''] : aboveZeroMsg; - if (parsedQuantity > pool.available) return [false, `Quantity must not be above ${pool.available}`]; + if (parsedQuantity > pool.available) return [false, __(`Quantity must not be above ${pool.available}`)]; if (parsedQuantity <= 0) return aboveZeroMsg; } else { - return [false, 'Please enter digits only']; + return [false, __('Please enter digits only')]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js` around lines 15 - 31, The quantityValidation function returns hardcoded English error messages that are not translatable. Wrap all string literals in the function with the __() localization function, including the strings in the aboveZeroMsg variable definition, the 'Please limit number to 10 digits' message, 'No pools available' message, the dynamic 'Quantity must not be above' template string, and the 'Please enter digits only' message. Ensure template literals with variables like pool.available are properly preserved within the __() calls to maintain interpolation functionality.
🧹 Nitpick comments (1)
webpack/components/OptionTooltip/index.js (1)
83-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: strengthen popover accessibility.
The button sets
aria-expandedbut there's noaria-controls/idlink to the popover, and the popover closes only on outside click or toggle (noEscape). Consider addingid/aria-controlsand anEscapehandler for keyboard users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webpack/components/OptionTooltip/index.js` around lines 83 - 87, The popover div with className "option-tooltip-popover" needs an id attribute, and the button that controls it needs an aria-controls attribute that references this id to properly link them for screen readers. Additionally, add a keyboard event handler to detect the Escape key and close the popover when pressed, ensuring keyboard users can dismiss it without relying on outside clicks or the toggle button. Use the popoverRef to help manage focus and state changes when handling the Escape key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webpack/components/OptionTooltip/index.js`:
- Line 78: The aria-label attribute on the button element in the OptionTooltip
component contains a hardcoded English string "Toggle column visibility" that is
not wrapped in the localization function. To fix this, wrap the aria-label value
with the __() function to make it translatable, and ensure that the __ function
is imported at the top of the index.js file from your localization library.
In `@webpack/scenes/Subscriptions/SubscriptionsPage.js`:
- Around line 66-75: The issue is that loadTableColumns is only called when a
matching SUBSCRIPTION_TABLE_NAME is found in the tables results, but when
loadTables returns empty results or no matching table is found, loadTableColumns
is never invoked and the Redux state remains with empty column arrays. To fix
this, add a fallback call to loadTableColumns with empty input outside the
nested conditional check so that the function is always called at least once in
the useEffect, either with the found subscriptionTable or with empty input to
trigger the default columns fallback behavior when no matching table exists.
---
Duplicate comments:
In
`@webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js`:
- Around line 15-31: The quantityValidation function returns hardcoded English
error messages that are not translatable. Wrap all string literals in the
function with the __() localization function, including the strings in the
aboveZeroMsg variable definition, the 'Please limit number to 10 digits'
message, 'No pools available' message, the dynamic 'Quantity must not be above'
template string, and the 'Please enter digits only' message. Ensure template
literals with variables like pool.available are properly preserved within the
__() calls to maintain interpolation functionality.
---
Nitpick comments:
In `@webpack/components/OptionTooltip/index.js`:
- Around line 83-87: The popover div with className "option-tooltip-popover"
needs an id attribute, and the button that controls it needs an aria-controls
attribute that references this id to properly link them for screen readers.
Additionally, add a keyboard event handler to detect the Escape key and close
the popover when pressed, ensuring keyboard users can dismiss it without relying
on outside clicks or the toggle button. Use the popoverRef to help manage focus
and state changes when handling the Escape key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fe28a87-c313-4318-9c31-eba9866a2e60
📥 Commits
Reviewing files that changed from the base of the PR and between bfd5755b8bfda6a87988eafe284972c7e3f840fd and c8289d4fb6b1b2504e1194c3d4b47ce7d29f58c3.
📒 Files selected for processing (19)
webpack/components/OptionTooltip/OptionTooltip.scsswebpack/components/OptionTooltip/__tests__/OptionTooltip.test.jswebpack/components/OptionTooltip/index.jswebpack/components/TooltipButton/TooltipButton.jswebpack/redux/reducers/index.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.jswebpack/scenes/Subscriptions/Details/SubscriptionDetails.jswebpack/scenes/Subscriptions/Details/__tests__/SubscriptionDetailProducts.test.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.scsswebpack/scenes/Subscriptions/SubscriptionsPage.jswebpack/scenes/Subscriptions/SubscriptionsPage.scsswebpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.jswebpack/scenes/Subscriptions/UpstreamSubscriptions/__tests__/UpstreamSubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
✅ Files skipped from review due to trivial changes (2)
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.js
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.scss
🚧 Files skipped from review as they are similar to previous changes (13)
- webpack/redux/reducers/index.js
- webpack/scenes/Subscriptions/SubscriptionsPage.scss
- webpack/scenes/Subscriptions/Details/tests/SubscriptionDetailProducts.test.js
- webpack/scenes/Subscriptions/tests/SubscriptionsPage.test.js
- webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
- webpack/components/OptionTooltip/tests/OptionTooltip.test.js
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/tests/UpstreamSubscriptionsPage.test.js
- webpack/components/OptionTooltip/OptionTooltip.scss
- webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetails.js
- webpack/components/TooltipButton/TooltipButton.js
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js`:
- Line 25: The validation error message in the return statement uses a template
literal inside the `__()` function, which prevents translation extraction tools
from finding the translatable string. Replace the template literal syntax with
sprintf() to separate the static translatable template from the dynamic
pool.available value. Import sprintf if not already available, then restructure
the error message call to use sprintf with a static format string containing a
placeholder, passing the pool.available value as an argument to sprintf, and
wrap the result appropriately for translation extraction. This ensures the
translatable portion remains static and detectable by gettext extraction tools.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ebf9120c-e4a7-44d3-b86f-d88944ce339b
📥 Commits
Reviewing files that changed from the base of the PR and between c8289d4fb6b1b2504e1194c3d4b47ce7d29f58c3 and 3d589be40ee03951e29d8b8ab8e4fd90ce54d3d7.
📒 Files selected for processing (19)
webpack/components/OptionTooltip/OptionTooltip.scsswebpack/components/OptionTooltip/__tests__/OptionTooltip.test.jswebpack/components/OptionTooltip/index.jswebpack/components/TooltipButton/TooltipButton.jswebpack/redux/reducers/index.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.jswebpack/scenes/Subscriptions/Details/SubscriptionDetails.jswebpack/scenes/Subscriptions/Details/__tests__/SubscriptionDetailProducts.test.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.scsswebpack/scenes/Subscriptions/SubscriptionsPage.jswebpack/scenes/Subscriptions/SubscriptionsPage.scsswebpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.jswebpack/scenes/Subscriptions/UpstreamSubscriptions/__tests__/UpstreamSubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
✅ Files skipped from review due to trivial changes (2)
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.scss
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js
🚧 Files skipped from review as they are similar to previous changes (14)
- webpack/scenes/Subscriptions/tests/SubscriptionsPage.test.js
- webpack/redux/reducers/index.js
- webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.js
- webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js
- webpack/components/TooltipButton/TooltipButton.js
- webpack/scenes/Subscriptions/Details/tests/SubscriptionDetailProducts.test.js
- webpack/components/OptionTooltip/OptionTooltip.scss
- webpack/components/OptionTooltip/tests/OptionTooltip.test.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetails.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/tests/UpstreamSubscriptionsPage.test.js
- webpack/scenes/Subscriptions/SubscriptionsPage.js
- webpack/scenes/Subscriptions/SubscriptionsPage.scss
69c1b20 to
8837d7d
Compare
|
Note The previously reviewed commits are no longer reachable (likely due to a force-push or rebase), so CodeRabbit is performing a full review instead of an incremental one. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.js (1)
22-28: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConfirm intended locale for
toLocaleDateString.Passing
undefinedas the locale uses the browser/runtime locale rather than the application's configured i18n locale, which can diverge from the rest of the UI's translated formatting. If app-locale consistency is required here, pass the active locale explicitly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.js` around lines 22 - 28, The date formatting in formatDate currently relies on the runtime default locale via toLocaleDateString(undefined, ...), which may not match the app’s configured i18n locale. Update formatDate in SubscriptionsTableSchema to use the active application locale explicitly (the same locale source used elsewhere in the UI) so the rendered subscription dates stay consistent with the rest of the translations and formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.js`:
- Around line 23-45: The SubscriptionDetailProductContent render logic is using
listItems.length to decide the branch, which can show empty cards for items
whose availableContent is empty. Update the component to filter listItems by
availableContent.length > 0 before rendering the Grid, and use that filtered
collection for both the map and the empty-state check so “No products are
enabled.” appears only when there are no products with content. Keep the change
localized to SubscriptionDetailProductContent and its listItems rendering logic.
---
Nitpick comments:
In
`@webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.js`:
- Around line 22-28: The date formatting in formatDate currently relies on the
runtime default locale via toLocaleDateString(undefined, ...), which may not
match the app’s configured i18n locale. Update formatDate in
SubscriptionsTableSchema to use the active application locale explicitly (the
same locale source used elsewhere in the UI) so the rendered subscription dates
stay consistent with the rest of the translations and formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e03eb9a-6f16-4e7d-9df2-db189858a529
📒 Files selected for processing (23)
webpack/components/OptionTooltip/OptionTooltip.scsswebpack/components/OptionTooltip/__tests__/OptionTooltip.test.jswebpack/components/OptionTooltip/index.jswebpack/components/TooltipButton/TooltipButton.jswebpack/redux/reducers/index.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.jswebpack/scenes/Subscriptions/Details/SubscriptionDetails.jswebpack/scenes/Subscriptions/Details/__tests__/SubscriptionDetailProducts.test.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.scsswebpack/scenes/Subscriptions/SubscriptionConstants.jswebpack/scenes/Subscriptions/SubscriptionsPage.jswebpack/scenes/Subscriptions/SubscriptionsPage.scsswebpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.jswebpack/scenes/Subscriptions/UpstreamSubscriptions/__tests__/UpstreamSubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/subscriptions.fixtures.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.jswebpack/scenes/Subscriptions/index.js
💤 Files with no reviewable changes (3)
- webpack/components/OptionTooltip/tests/OptionTooltip.test.js
- webpack/components/OptionTooltip/OptionTooltip.scss
- webpack/components/OptionTooltip/index.js
✅ Files skipped from review due to trivial changes (2)
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.scss
- webpack/scenes/Subscriptions/SubscriptionConstants.js
🚧 Files skipped from review as they are similar to previous changes (9)
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.js
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.js
- webpack/redux/reducers/index.js
- webpack/scenes/Subscriptions/Details/tests/SubscriptionDetailProducts.test.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/tests/UpstreamSubscriptionsPage.test.js
- webpack/components/TooltipButton/TooltipButton.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetails.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
webpack/components/pf3Table/formatters/EntitlementsInlineEditFormatter.js (1)
66-78: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the edit field mounted during quantity reloads
availableQuantitiesis set back tonullonSUBSCRIPTIONS_QUANTITIES_REQUEST, so this ternary can switch back to the spinner mid-edit and unmount the uncontrolledFormControlbeforeonBlursaves the draft. Keep the input mounted through reloads or persist the draft in state first.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webpack/components/pf3Table/formatters/EntitlementsInlineEditFormatter.js` around lines 66 - 78, The edit field in EntitlementsInlineEditFormatter should stay mounted while quantity data reloads instead of switching back to the Spinner when availableQuantities becomes null. Update the conditional around the FormGroup/FormControl so the uncontrolled input remains rendered during SUBSCRIPTIONS_QUANTITIES_REQUEST, or move the draft value into state before reload; use the EntitlementsInlineEditFormatter render path and the onBlur handler as the key places to adjust.webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js (1)
89-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
disableAddButtonon the actual button.Line 92 blocks navigation for
disableAddButton, but Line 99 only disables the child button fordisableManifestActions. That leaves the control looking enabled when the action is intentionally unavailable.Suggested fix
<TooltipButton tooltipId="add-subscriptions-button-tooltip" tooltipText={disableManifestReason} tooltipPlacement="top" title={__('Add subscriptions')} - disabled={disableManifestActions} + disabled={disableManifestActions || disableAddButton} variant="primary" />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js` around lines 89 - 101, The SubscriptionsToolbar add control is only using disableManifestActions on the TooltipButton, so the button can still appear enabled when disableAddButton is true. Update the add-subscriptions control in SubscriptionsToolbar so the actual TooltipButton reflects the same disabled state used by LinkContainer, combining disableManifestActions and disableAddButton. Keep the existing symbols LinkContainer and TooltipButton in sync so the navigation blocker and visible button state match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/DeleteDialog.js`:
- Around line 35-38: The DeleteDialog confirmation text is using a hardcoded
“subscription(s)” string, which cannot be localized correctly. Update the
message in DeleteDialog to use the plural-aware translation helper ngettext
(n__) with separate singular and plural forms, and keep the selectedRows.length
count driving the choice so the dialog text is correctly translated based on the
number of subscriptions.
In
`@webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/UpdateDialog.js`:
- Around line 49-53: The entitlement-count message in UpdateDialog should not
hardcode English pluralization with “entitlement(s)”; update the string used in
the surrounding `sprintf` call to use `n__` so translators can provide the
correct singular/plural form based on `quantityLength`. Locate the message in
`SubscriptionsTable`’s `UpdateDialog` component and adjust the translation call
so the count-aware text is driven by `n__` rather than a plain `__` string.
In
`@webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js`:
- Around line 136-169: The kebab menu in SubscriptionsToolbar should honor the
same manifest-action disabling rules as the previous toolbar actions. Update the
Manage manifest DropdownItem and the Delete DropdownItem so they are disabled
whenever disableManifestActions is true, and ensure their tooltipProps use the
manifest-specific disable reason instead of always showing disableDeleteReason.
Keep the existing click handlers and state wiring in SubscriptionsToolbar
consistent with the disable contract so the PF5 dropdown matches the old
behavior.
In `@webpack/scenes/Subscriptions/SubscriptionsPage.scss`:
- Line 46: The `fill: currentColor;` declaration in `SubscriptionsPage.scss` is
tripping Stylelint’s keyword-case rule, so update the `currentColor` value to
the casing expected by the linter in this stylesheet. Make the fix directly in
the `fill` rule so the `SubscriptionsPage` styles pass CI linting without
changing the selector or surrounding rules.
---
Outside diff comments:
In `@webpack/components/pf3Table/formatters/EntitlementsInlineEditFormatter.js`:
- Around line 66-78: The edit field in EntitlementsInlineEditFormatter should
stay mounted while quantity data reloads instead of switching back to the
Spinner when availableQuantities becomes null. Update the conditional around the
FormGroup/FormControl so the uncontrolled input remains rendered during
SUBSCRIPTIONS_QUANTITIES_REQUEST, or move the draft value into state before
reload; use the EntitlementsInlineEditFormatter render path and the onBlur
handler as the key places to adjust.
In
`@webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.js`:
- Around line 89-101: The SubscriptionsToolbar add control is only using
disableManifestActions on the TooltipButton, so the button can still appear
enabled when disableAddButton is true. Update the add-subscriptions control in
SubscriptionsToolbar so the actual TooltipButton reflects the same disabled
state used by LinkContainer, combining disableManifestActions and
disableAddButton. Keep the existing symbols LinkContainer and TooltipButton in
sync so the navigation blocker and visible button state match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86038c81-dfb8-4f93-a399-3244ad1ce148
📒 Files selected for processing (29)
webpack/components/LoadingState/LoadingState.jswebpack/components/OptionTooltip/OptionTooltip.scsswebpack/components/OptionTooltip/__tests__/OptionTooltip.test.jswebpack/components/OptionTooltip/index.jswebpack/components/TooltipButton/TooltipButton.jswebpack/components/pf3Table/formatters/EntitlementsInlineEditFormatter.jswebpack/redux/reducers/index.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProductContent.jswebpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.jswebpack/scenes/Subscriptions/Details/SubscriptionDetails.jswebpack/scenes/Subscriptions/Details/__tests__/SubscriptionDetailProducts.test.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.jswebpack/scenes/Subscriptions/Manifest/ManageManifestModal.scsswebpack/scenes/Subscriptions/SubscriptionConstants.jswebpack/scenes/Subscriptions/SubscriptionsPage.jswebpack/scenes/Subscriptions/SubscriptionsPage.scsswebpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.jswebpack/scenes/Subscriptions/UpstreamSubscriptions/__tests__/UpstreamSubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/SubscriptionsPage.test.jswebpack/scenes/Subscriptions/__tests__/subscriptions.fixtures.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/DeleteDialog.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/InputsErrorsDialog.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/UnsavedChangesDialog.jswebpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/UpdateDialog.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.jswebpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.jswebpack/scenes/Subscriptions/index.js
💤 Files with no reviewable changes (3)
- webpack/components/OptionTooltip/tests/OptionTooltip.test.js
- webpack/components/OptionTooltip/OptionTooltip.scss
- webpack/components/OptionTooltip/index.js
✅ Files skipped from review due to trivial changes (3)
- webpack/scenes/Subscriptions/Manifest/ManageManifestModal.scss
- webpack/scenes/Subscriptions/tests/subscriptions.fixtures.js
- webpack/scenes/Subscriptions/SubscriptionConstants.js
🚧 Files skipped from review as they are similar to previous changes (13)
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProducts.js
- webpack/redux/reducers/index.js
- webpack/scenes/Subscriptions/Details/tests/SubscriptionDetailProducts.test.js
- webpack/components/TooltipButton/TooltipButton.js
- webpack/scenes/Subscriptions/index.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/tests/UpstreamSubscriptionsPage.test.js
- webpack/scenes/Subscriptions/components/SubscriptionsTable/SubscriptionsTableSchema.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetailProduct.js
- webpack/scenes/Subscriptions/components/SubscriptionsToolbar/SubscriptionsToolbar.test.js
- webpack/scenes/Subscriptions/tests/SubscriptionsPage.test.js
- webpack/scenes/Subscriptions/Details/SubscriptionDetails.js
- webpack/scenes/Subscriptions/UpstreamSubscriptions/UpstreamSubscriptionsPage.js
- webpack/scenes/Subscriptions/SubscriptionsPage.js
| {sprintf( | ||
| __('Are you sure you want to delete %s subscription(s)? This action will remove the subscription(s) and refresh your manifest. All systems using these subscription(s) will lose them and also may lose access to updates and Errata.'), | ||
| selectedRows.length, | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which pluralization helper is used in this repo and find similar strings.
rg -n "ngettext|dngettext|plural" webpack app
rg -n "subscription\\(s\\)|entitlement\\(s\\)" webpack/scenes/Subscriptions
rg -n "common/I18n" webpack appRepository: Katello/katello
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## DeleteDialog.js\n'
sed -n '1,220p' webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/DeleteDialog.js
printf '\n## Nearby pluralization patterns in Subscriptions scene\n'
rg -n "ngettext|n__|TranslatedPlural|pluralize\\(|subscription\\(s\\)|subscription(s)" webpack/scenes/Subscriptions webpack/components -g '!app/assets/**'
printf '\n## I18n imports in relevant files\n'
rg -n "foremanReact/common/I18n|common/I18n" webpack/scenes/Subscriptions webpack/components -g '!app/assets/**'Repository: Katello/katello
Length of output: 46191
Use a plural-aware translation here. subscription(s) can’t be localized correctly; switch this dialog to ngettext (n__) with separate singular/plural forms.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/DeleteDialog.js`
around lines 35 - 38, The DeleteDialog confirmation text is using a hardcoded
“subscription(s)” string, which cannot be localized correctly. Update the
message in DeleteDialog to use the plural-aware translation helper ngettext
(n__) with separate singular and plural forms, and keep the selectedRows.length
count driving the choice so the dialog text is correctly translated based on the
number of subscriptions.
| <p> | ||
| {sprintf( | ||
| __("You're making changes to %s entitlement(s)"), | ||
| quantityLength, | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which pluralization helper is used in this repo and find similar strings.
rg -n "ngettext|dngettext|plural" webpack app
rg -n "subscription\\(s\\)|entitlement\\(s\\)" webpack/scenes/Subscriptions
rg -n "common/I18n" webpack appRepository: Katello/katello
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/UpdateDialog.js"
echo "--- file outline ---"
ast-grep outline "$FILE" --view expanded || true
echo "--- relevant lines ---"
sed -n '1,220p' "$FILE" | cat -n
echo "--- pluralization usage in nearby subscriptions dialogs ---"
rg -n "ngettext|n__|pluralize|entitlement\\(s\\)|subscription\\(s\\)" webpack/scenes/Subscriptions webpack/components | head -n 200Repository: Katello/katello
Length of output: 4752
Use n__ for the entitlement count.
entitlement(s) bakes English pluralization into the message, so translators can’t choose the right form for quantityLength.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@webpack/scenes/Subscriptions/components/SubscriptionsTable/components/Dialogs/UpdateDialog.js`
around lines 49 - 53, The entitlement-count message in UpdateDialog should not
hardcode English pluralization with “entitlement(s)”; update the string used in
the surrounding `sprintf` call to use `n__` so translators can provide the
correct singular/plural form based on `quantityLength`. Locate the message in
`SubscriptionsTable`’s `UpdateDialog` component and adjust the translation call
so the count-aware text is driven by `n__` rather than a plain `__` string.
There was a problem hiding this comment.
Since we're working on an effort to make foreman+plugins look consistent, were hoping to control all the spacings using PageLayout/TableIndexPage component. Using custom css means you'll have to keep updating this file if we change the padding defaults
There was a problem hiding this comment.
Thanks for the feedback! Refactored to use PageLayout
| margin-right: 8px; | ||
| } | ||
|
|
||
| // Match PF3 button sizing for Refresh and Delete buttons |
There was a problem hiding this comment.
If this is being updated to pf5, why match the styles to pf3?
There was a problem hiding this comment.
Yes, I forgot to change this in the latest commit, will update it.
There was a problem hiding this comment.
Removed the custom CSS.
qcjames53
left a comment
qcjames53
left a comment
There was a problem hiding this comment.
Hey Pavan! Overall I only had a few suggestions for this PR. It's looking good!
Looking into deleted files/components revealed no issues. Manifest delete and import work fine and look visually okay to me + provide progress feedback while blocking page actions. Binding entitlements shows progress as well and similarly blocks actions. All the header buttons work in my testing. Table contents look appropriate aside from one comment below. Pagination works well. The spinners function properly.
| console.error('Failed to load table column preferences:', error); | ||
| loadTableColumns({ columns: SUBSCRIPTION_TABLE_DEFAULT_COLUMNS }); | ||
| } | ||
| }, [userColumns, loadTableColumns]); |
There was a problem hiding this comment.
I'm not so sure loadTableColumns should be in the dependency array. I think it can be safely removed.
There was a problem hiding this comment.
Claude wants me to add this context: Line 83: loadTableColumns can be safely removed from the dependency array - it comes from bindActionCreators which creates a stable reference.
There was a problem hiding this comment.
Removed loadTableColumns from the dependency array.
| }, []); | ||
|
|
||
| // Initial mount: reset tasks and load data if organization exists | ||
| // Initial mount: reset tasks only |
There was a problem hiding this comment.
Dropped the comment.
| onOpenChange={isOpen => setIsKebabOpen(isOpen)} | ||
| > | ||
| <DropdownList> | ||
| {disableManifestActions ? ( |
There was a problem hiding this comment.
This ternary operator should be removed and replaced with the bottom case. The user should always be able to click "Manage manifests". I got myself into a pickle with an expired manifest and couldn't remove it through the UI.
There was a problem hiding this comment.
Removed the ternary. "Manage manifest" is now always clickable regardless of manifest connection status.
| }, | ||
| }, | ||
| { | ||
| property: 'product_id', |
There was a problem hiding this comment.
Would you please remove the candlepin product ID from the SKU column for local products? It's something we probably shouldn't be displaying to users and this is a good time to correct that.
There was a problem hiding this comment.
SKU column now only shows for upstream subscriptions, hiding internal Candlepin IDs for local products.
There was a problem hiding this comment.
Since you fixed other capitalization I figured this would be a good add.
| subscriptionsService: <a href={SUBSCRIPTIONS_SERVICE_DOC_URL} target="_blank" rel="noreferrer">{__('subscriptions service')}</a>, |
There was a problem hiding this comment.
I feel like this needs a reword. The line break doesn't really look good anyways. This is just a first pass; feel free to change anything I suggest.
| defaultMessage={__(`This page shows subscriptions available from this organization's subscription manifest alongside this organization's locally-hosted products. Learn more about subscriptions and entitlement management with the {subscriptionsService}.`)} |
There was a problem hiding this comment.
Applied your suggestion.
|
|
||
| <SubscriptionsToolbar | ||
| <> | ||
| {/* Header Section */} |
There was a problem hiding this comment.
Let's drop these JSX comments.
There was a problem hiding this comment.
Dropped all the JSX comments.
| </Title> | ||
| </PageSection> | ||
|
|
||
| {/* Modals */} |
There was a problem hiding this comment.
Let's drop these JSX comments.
| closeModal={() => setIsManageManifestModalOpen(false)} | ||
| /> | ||
|
|
||
| {/* Table Section (includes toolbar) */} |
There was a problem hiding this comment.
Let's drop these JSX comments.
| {canManageSubscriptionAllocations && ( | ||
| <> | ||
| <Divider key="divider" /> | ||
| {(disableManifestActions || disableDeleteButton) ? ( |
There was a problem hiding this comment.
Same situation with disableManifestActions. I believe delete does not require manifest connection so it should always be available.
There was a problem hiding this comment.
Removed the disableManifestActions check so delete is always available regardless of manifest connection status.
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
|
Unchecking all columns in the Manage Columns modal causes the Save button to hang indefinitely. Fixed by making the Name column required (always selected and disabled), following the same pattern as All Hosts. |
qcjames53
left a comment
qcjames53
left a comment
There was a problem hiding this comment.
Thanks for the fixes. Good call on making the name column required. A few questions + concerns.
| if (!dateString) return <td>NA</td>; | ||
| const date = new Date(dateString); | ||
| if (Number.isNaN(date.getTime())) return <td>NA</td>; |
There was a problem hiding this comment.
Could you make these return <td>—</td> please? I think it's probably the better option over N/A for a malformatted date.
There was a problem hiding this comment.
Changed to <td>—</td> for both missing and malformatted dates.
| formatters: [ | ||
| (value, { rowData }) => { | ||
| if (!rowData.upstream_pool_id) { | ||
| return <td />; |
There was a problem hiding this comment.
Sorry to double request something but I'd love if we could do <td>—</td> here as well. Would you mind moving this into a formatter method like the other columns?
There was a problem hiding this comment.
Created skuFormatter function that returns <td>—</td> for local products.
| @@ -89,40 +102,42 @@ export const createSubscriptionsTableSchema = ( | |||
| cell: { | |||
| formatters: [cellFormatter], | |||
There was a problem hiding this comment.
Would we also be able to do <td>—</td> for this field if it's blank?
There was a problem hiding this comment.
Created textFormatter function that returns <td>—</td> for blank/empty values.
|
|
||
| .table-view-pf-select { | ||
| width: 50px; | ||
| .editable { |
There was a problem hiding this comment.
Is it intentional that these bottom three blocks are global? We should probably scope to something on the page.
There was a problem hiding this comment.
Not intentional! Missed that during cleanup.
| margin-bottom: 0; | ||
| // Manage columns - show only icon, hide text | ||
| // Remove margin from the toolbar item wrapper that contains column selector | ||
| [class*="toolbar__item"]:has(#column-selector) { |
There was a problem hiding this comment.
:has is pretty new and from what I can tell has only been supported in browsers for the last 2 or so years. If we can avoid using it that would be awesome.
Claude says pf-v5-c-toolbar__item:has(#column-selector) is a decent alternative.
There was a problem hiding this comment.
Replaced with custom class .subscriptions-column-selector to avoid browser compatibility issues.
| #column-selector { | ||
| button { | ||
| // Add explicit horizontal padding to match toolbar item spacing | ||
| padding-left: 16px; |
There was a problem hiding this comment.
Claude says these should probably use pf5 spacing vars: var(--pf-v5-global--spacer--md); instead of hardcoded px values.
There was a problem hiding this comment.
Fixed! Replaced 16px with var(--pf-v5-global--spacer--md).
| width: 50px; | ||
| .editable { | ||
| &.changed { | ||
| font-weight: bold; |
There was a problem hiding this comment.
I can't get this bold to render no matter what I do on the table. Can you point me in the right direction?
There was a problem hiding this comment.
The changed class is pre-existing functionality for inline entitlement editing. To see it on the main Subscriptions page: click on an editable entitlement number in the "Entitlements" column, modify the value, and the cell will become bold while editing (before saving).
There was a problem hiding this comment.
Oh I see the effect is very slight. Thank you!
| } | ||
| } | ||
|
|
||
| span.fa.fa-minus::before { |
There was a problem hiding this comment.
This can be removed since the virt_who column shows true/false, not fa-minus.
There was a problem hiding this comment.
@parthaa is going to get back to us in a bit about whether https://redhat.atlassian.net/browse/SAT-32071 will make Katello 4.21 or not. If it does, most of this scss file can go away, right? Should we just pare down to the minimum styling and wait for the pf5 table?
| float: left; | ||
| font-weight: bold; | ||
| } | ||
| .pf-table-inline-edit { |
There was a problem hiding this comment.
Not sure where to put this feedback but is this correct button placement? I can't access the mock up and wanted to confirm with you. It looks a little goofy imho.
There was a problem hiding this comment.
I had the same thought! The positioning does look a bit odd. This is pre-existing behavior though; since this PR is just migrating components from PF3 to PF5, I kept the existing placement.
pr-processor
Bot
added
Needs re-review
and removed
Waiting on contributor
Needs re-review
labels
| closeModal={() => setIsManageManifestModalOpen(false)} | ||
| /> | ||
|
|
||
| <PageLayout |
There was a problem hiding this comment.
Please also send a header, its used for the browser tab title
qcjames53
left a comment
qcjames53
left a comment
There was a problem hiding this comment.
This looks really good. One ask and I think we'll be good to merge!
| (value, { rowData }) => { | ||
| // Show NA if virt_who is null/undefined (missing data) | ||
| if (rowData.virt_who === null || rowData.virt_who === undefined) { | ||
| return <td>NA</td>; |
There was a problem hiding this comment.
Would you mind making this an em dash?
There was a problem hiding this comment.
Implemented the suggested change.
| <LoadingState loading={upstreamSubscriptions.loading} loadingText={__('Loading')}> | ||
| <Grid hasGutter> | ||
| <GridItem span={12}> | ||
| <Table |
There was a problem hiding this comment.
We accidentally dropped the ouiaId on this table. Please add it back :)
ouiaId="upstream-subscriptions-table"
There was a problem hiding this comment.
Added ouia-id back.
qcjames53
left a comment
qcjames53
left a comment
There was a problem hiding this comment.
Thanks for all the changes. This looks great to me!
3 participants
What are the changes introduced in this pull request?
This PR migrates the Subscriptions page from PF3 to PF5, updating all components, styles, and tests. Key changes include migrating PF3 components (Grid/Row/Col, TabContainer, ListView, OverlayTrigger, MessageDialog, Spinner, Button) to their PF5 equivalents (Grid/GridItem, Tabs/Tab, Card, manual popover), converting class components to function components with hooks (SubscriptionDetails, UpstreamSubscriptionsPage), and migrating tests from Enzyme to React Testing Library with semantic queries. The Spinner migration from PF3 to PF5 spans shared components (LoadingState, EntitlementsInlineEditFormatter) and Subscriptions-specific components (ManageManifestModal), automatically upgrading 9+ places across Katello. All MessageDialog components are migrated to PF5 Modal with actions arrays.
The column management is migrated from the custom OptionTooltip component to Foreman's standard ColumnSelector modal with persistence via table_preferences API. The page layout uses Foreman's PageLayout component with customHeader and customToolbar props for consistent spacing across Foreman and plugins. The migration includes updated PF5 styling with sentence case column headers, locale-aware date formatting, consistent toolbar icons, and the complete removal of the unused OptionTooltip component.
Considerations taken when implementing this change?
The migration follows React best practices with correct useEffect dependencies, immutable state updates, and appropriate useCallback usage. Tests use semantic queries (getByRole, getByLabelText) for better accessibility and maintainability. Column management follows Foreman's existing patterns (matching All Hosts page), eliminates duplicate API calls for 50% performance improvement, adds error handling with graceful fallback to defaults, and uses PF5 CSS custom properties for consistent theming. The Spinner migration removes the PF3 loading prop pattern in favor of conditional rendering ({condition && }), ensuring consistent behavior across shared and Subscriptions-specific components. Using PageLayout ensures spacing automatically matches Foreman's standards and will pick up future updates without manual changes.
What are the testing steps for this pull request?
Summary by CodeRabbit
variantoption for tooltip buttons, including improved Refresh/Delete styling.