Fixes #39047 - replace useForemanModal in BulkActions by Lukshio · Pull Request #11626 · Katello/katello

Lukshio · 2026-01-30T16:18:11Z

Replacement of useForemanModal hook before its deprecation

Summary by Sourcery

Replace deprecated Foreman modal usage in host bulk actions with a shared custom bulk modal state mechanism.

Enhancements:

Introduce a centralized bulk modal state helper to manage open/close state for host bulk action modals.
Update host actions bar and all bulk action modals to use the new bulk modal state instead of the deprecated useForemanModal hook.

sourcery-ai

Hey - I've left some high level feedback:

The modal IDs (e.g. 'bulk-packages-wizard', 'bulk-errata-wizard', etc.) are now duplicated across ActionsBar and the various bulk modal scenes; consider extracting them into shared constants to avoid typo bugs and make future refactors safer.
In BulkAssignCVEnvsModalScene the indentation around allowMultipleContentViews and the if (!orgId) return null; block is inconsistent with the surrounding code, which makes the control flow harder to read; aligning these lines would improve clarity.
The global modalState and listeners objects in bulkModalState.js introduce hidden shared state; if these components are ever mounted/unmounted dynamically or used in tests, consider encapsulating this in a React context or a more explicit state container to avoid subtle cross-component coupling.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The modal IDs (e.g. 'bulk-packages-wizard', 'bulk-errata-wizard', etc.) are now duplicated across ActionsBar and the various bulk modal scenes; consider extracting them into shared constants to avoid typo bugs and make future refactors safer.
- In `BulkAssignCVEnvsModalScene` the indentation around `allowMultipleContentViews` and the `if (!orgId) return null;` block is inconsistent with the surrounding code, which makes the control flow harder to read; aligning these lines would improve clarity.
- The global `modalState` and `listeners` objects in `bulkModalState.js` introduce hidden shared state; if these components are ever mounted/unmounted dynamically or used in tests, consider encapsulating this in a React context or a more explicit state container to avoid subtle cross-component coupling.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

MariaAga

Thanks! except the extra empty line in global_index.js it looks good to me.
Tested with packit that all the modal open/close and show up as expected

jeremylenz

Comments below, and one general question: Why is useBulkModalOpen in Katello? If there are any other plugins that need to keep track of modal state like this, useBulkModalOpen should really be in Foreman.

jeremylenz · 2026-02-04T21:25:17Z

@@ -0,0 +1,23 @@
+import { useEffect, useState } from 'react';
+
+const modalState = {};


This is kind of a crazy solution but after thinking about it I think it's right. Refs are component-scoped so don't fit here. Context is too tied to the React tree so also doesn't fit. I guess vanilla JS to the rescue :D

Unfortunately, Katello is the only one that uses the useForemanModal function to manage modal states. Because the components are mounted using globalFill, there are not many options. The easiest way would be to mount it in the ActionBar component. However, it gets dismounted when the user clicks on the menu and closes it. Therefore, it should either stay open or the plugin mount should be refactored. Neither option is ideal because it requires changes to the core. The same problem occurs with the context; it also gets dismounted. To avoid using Redux, this may be the only way to deal with it.

Another option would be to refactor the Katello modals so they (or their parent components) manage their own open states.

I tried to find some way to do it like that. But I always ended up with this solution. I always end up on the same issue with mounts and opening from ActionBar menu.

jeremylenz

One more thing to note - in testing another bug with @nofaralfasi we noticed that moving away from useForemanModal moves your modal's API request earlier:

useForemanModal: API request is only made when modal is opened, even without a conditional check
Now: API request is made immediately on page load, unless you add a conditional in your modal

This may or may not have user impact.

Lukshio · 2026-02-05T11:50:25Z

One more thing to note - in testing another bug with @nofaralfasi we noticed that moving away from useForemanModal moves your modal's API request earlier:

useForemanModal: API request is only made when modal is opened, even without a conditional check Now: API request is made immediately on page load, unless you add a conditional in your modal

This may or may not have user impact.

This behavior could be caused by the filter function instead of delete. Now I partially changed the behavior using delete, and I couldn't recreate the API issue.

Lukshio · 2026-02-05T16:07:46Z

Fix lint errors + rename function to openBulkModal

Lukshio · 2026-02-06T16:02:17Z

Thank you for all the reviews, is there anything to be edited?

jeremylenz

Thanks @Lukshio

Works fine in my testing. I suppose this is the best alternative for now. ACK 👍

nadjaheitmann · 2026-03-02T19:55:15Z

Comments below, and one general question: Why is useBulkModalOpen in Katello? If there are any other plugins that need to keep track of modal state like this, useBulkModalOpen should really be in Foreman.

Nice addition! May I also ask why this is not directly in Foreman? I'd love to have this for another PR and I'd hate to duplicate the code: theforeman/foreman_puppet#429

jeremylenz · 2026-03-02T20:04:51Z

If the best solution is to use the existing code, I'd say let's remove it from Katello and put it in Foreman.

nadjaheitmann · 2026-03-03T07:36:23Z

If the best solution is to use the existing code, I'd say let's remove it from Katello and put it in Foreman.

I can test it. I have basically copied the Katello logic to the foreman_puppet PR, so I assume it will work there as well.

nadjaheitmann · 2026-03-03T12:45:15Z

@jeremylenz @Lukshio I tested the 'useBulkModalOpen' in my foreman_puppet PR and it works very well. I copied the code manually to Foreman and imported it from there. +1 for moving this to Foreman directly! Would be very much appreciated.

jeremylenz · 2026-03-03T13:48:21Z

If someone would open a PR adding it to Foreman and another removing from Katello, I would approve them :)

nadjaheitmann · 2026-03-04T07:46:56Z

I have created theforeman/foreman#10893 in Foreman. It probably also makes sense to directly apply the fix there rather than just copying over the code...

Edit: Just found that one :) theforeman/foreman#10834

Lukshio · 2026-03-04T13:05:10Z

Hi, @nadjaheitmann @jeremylenz Thank you for bringing it up. I will review the PR for the core and then, as part of this process, create a new issue and PR to change the import in Katello.

The fix was originally implemented in Katello but can also be used for other plugins, so it makes more sense to have it in Foreman core: Katello/katello#11626

pr-processor Bot added the Not yet reviewed label Jan 30, 2026

sourcery-ai Bot reviewed Jan 30, 2026

View reviewed changes

pr-processor Bot removed the Not yet reviewed label Jan 30, 2026

Lukshio force-pushed the remove_useForemanModal_Bulk branch 2 times, most recently from 44474c9 to 5da1e04 Compare January 30, 2026 16:55

MariaAga reviewed Feb 3, 2026

View reviewed changes

Comment thread webpack/global_index.js Outdated

MariaAga approved these changes Feb 3, 2026

View reviewed changes

Lukshio force-pushed the remove_useForemanModal_Bulk branch from 5da1e04 to 0aea5a0 Compare February 4, 2026 10:31

lfu reviewed Feb 4, 2026

View reviewed changes

Comment thread webpack/components/extensions/Hosts/BulkActions/bulkModalState.js Outdated

jeremylenz reviewed Feb 4, 2026

View reviewed changes

Lukshio force-pushed the remove_useForemanModal_Bulk branch from 0aea5a0 to 050215b Compare February 5, 2026 12:06

jeremylenz reviewed Feb 5, 2026

View reviewed changes

Comment thread webpack/components/extensions/Hosts/BulkActions/bulkModalState.js Outdated

Comment thread webpack/components/extensions/Hosts/BulkActions/bulkModalState.js

Comment thread webpack/components/extensions/Hosts/BulkActions/bulkModalState.js Outdated

Fixes #39047 - replace useForemanModal in BulkActions

04bf199

Lukshio force-pushed the remove_useForemanModal_Bulk branch from 050215b to 04bf199 Compare February 5, 2026 16:07

jeremylenz approved these changes Feb 6, 2026

View reviewed changes

jeremylenz merged commit 7bfca4b into Katello:master Feb 6, 2026
24 of 26 checks passed

jeremylenz mentioned this pull request Mar 2, 2026

Draft: Add Puppet proxy (CA) bulk actions theforeman/foreman_puppet#429

Open

nadjaheitmann mentioned this pull request Mar 4, 2026

Fixes #39134 - Add bulk modal state helper to core theforeman/foreman#10893

Merged

Lukshio deleted the remove_useForemanModal_Bulk branch May 4, 2026 14:10

		@@ -0,0 +1,23 @@
		import { useEffect, useState } from 'react';

		const modalState = {};

Conversation

Lukshio commented Jan 30, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MariaAga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jeremylenz left a comment

Choose a reason for hiding this comment

Uh oh!

jeremylenz Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

Lukshio Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

jeremylenz Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukshio Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jeremylenz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukshio commented Feb 5, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lukshio commented Feb 5, 2026

Uh oh!

Lukshio commented Feb 6, 2026

Uh oh!

jeremylenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nadjaheitmann commented Mar 2, 2026

Uh oh!

jeremylenz commented Mar 2, 2026

Uh oh!

nadjaheitmann commented Mar 3, 2026

Uh oh!

nadjaheitmann commented Mar 3, 2026

Uh oh!

jeremylenz commented Mar 3, 2026

Uh oh!

nadjaheitmann commented Mar 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Lukshio commented Mar 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Lukshio commented Jan 30, 2026 •

edited by sourcery-ai Bot

Loading

jeremylenz Feb 5, 2026 •

edited

Loading

jeremylenz left a comment •

edited

Loading

nadjaheitmann commented Mar 4, 2026 •

edited

Loading