Fixes #38900 - Convert Host Collections list/details page to React

[chris1984]

What are the changes introduced in this pull request?

This PR migrates the Host Collections feature from AngularJS to React with PatternFly 5, accessible at /labs/host_collections.

Features implemented:

• List page with search, filter, pagination, and sorting
• Create, copy, and delete host collection functionality
• Details page with tabbed interface (Details and Hosts tabs)
• Inline editing for name, description, and host limit
• Host management (add/remove hosts from collections)
• Bulk selection with SelectAllCheckbox integration
• Permission-based action visibility
• Toast notifications for success/error states

Considerations taken when implementing this change?

• Used TableIndexPage from Foreman for consistency with other list/table pages
• Implemented Redux infrastructure (actions, reducers, selectors) for state management
• Fixed several API endpoint issues discovered during migration (copy endpoint parameters, hosts API path)
• Added to /labs menu rather than replacing AngularJS version immediately to allow gradual rollout
• Hash-based tab navigation enables direct linking to specific tabs
• InlineEdit component fixes ensure fields populate with current values

What are the testing steps for this pull request?

1. Navigate to /labs/host_collections to access the React version
2. List page: Verify search, filter, pagination, and sorting work correctly
3. Create: Click "Create Host Collection" and create a new collection
4. Details page: Click on a collection name to view details
5. Inline editing: Edit name, description, and host limit (test unlimited checkbox)
6. Hosts tab: Add and remove hosts from the collection
7. Bulk selection: Test select all/page/none functionality
8. Copy: Use Actions → Copy and verify navigation to new collection
9. Delete: Use Actions → Delete and verify return to list page

Summary by Sourcery

Convert the Host Collections feature to a React/PatternFly 5 implementation under the labs menu, providing a unified list and details experience with CRUD operations, inline edits, host membership management, and permission-aware actions backed by Redux state management and enhanced API integration.

New Features:

• Migrate Host Collections list and details pages from AngularJS to React using PatternFly 5 under /labs/host_collections
• Implement list page with search, filter, pagination, sorting, create, copy, and delete actions with permission-based visibility
• Add details page with tabbed interface (Details and Hosts) supporting inline editing of name, description, and host limit

Enhancements:

• Introduce Redux infrastructure (actions, reducers, selectors) for host collections and fix API endpoint issues discovered during migration
• Enable hash-based tab navigation and integrate Foreman’s TableIndexPage and SelectAllCheckbox for bulk selection workflows
• Surface success and error states via toast notifications across create, update, copy, add/remove hosts, and delete operations

Tests:

• Add unit tests for the React Host Collections list page to verify API calls and empty state rendering

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `webpack/scenes/HostCollections/Details/HostsTab/HostsTab.js:65-73` </location>
<code_context>
+  };
+
+  // Restrict search to only hosts in this collection
+  const restrictedSearchQuery = (userSearch) => {
+    const filter = `host_collection_id=${hostCollectionId}`;
+    const trimmedSearch = userSearch?.trim() ?? '';
+    if (!!trimmedSearch && !trimmedSearch.includes(filter)) {
+      return `${filter} and ${trimmedSearch}`;
+    }
+    return filter;
+  };
+
</code_context>

<issue_to_address>
**suggestion:** The restrictedSearchQuery function may not handle complex user search queries robustly.

Prepending the filter may cause issues with queries containing logical operators or parentheses. Consider wrapping the user input in parentheses or using a more reliable method to combine filters.

```suggestion
  // Restrict search to only hosts in this collection
  const restrictedSearchQuery = (userSearch) => {
    const filter = `host_collection_id=${hostCollectionId}`;
    const trimmedSearch = userSearch?.trim() ?? '';
    if (!!trimmedSearch && !trimmedSearch.includes(filter)) {
      // Wrap user search in parentheses to preserve logical grouping
      return `${filter} and (${trimmedSearch})`;
    }
    return filter;
  };
```
</issue_to_address>

### Comment 2
<location> `webpack/scenes/HostCollections/Create/CreateHostCollectionForm.js:49-50` </location>
<code_context>
+      type: 'success',
+      message: __('Host collection copied successfully'),
+    }));
+    onClose();
+    window.location.href = `/labs/host_collections/${data.id}`;
+  };
</code_context>

<issue_to_address>
**suggestion:** Using window.location.reload after creation may be disruptive to user experience.

Instead of reloading, update the local state or redirect to the new host collection page to maintain application state and improve user experience.

Suggested implementation:

```javascript
    // Use React Router's navigation to redirect without reloading the page
    navigate(`/labs/host_collections/${data.id}`);

```

1. Make sure to import `useNavigate` from `react-router-dom` at the top of the file:
   `import { useNavigate } from 'react-router-dom';`
2. Initialize the `navigate` function in your component:
   `const navigate = useNavigate();`
3. If you are not using React Router v6+, you may need to use `useHistory` and `history.push` instead.
4. Remove any other instances of `window.location.reload` or `window.location.href` in the file to ensure navigation is handled via React Router.
</issue_to_address>

### Comment 3
<location> `webpack/scenes/HostCollections/Copy/CopyHostCollectionModal.js:19-30` </location>
<code_context>
+      message: __('Host collection copied successfully'),
+    }));
+    onClose();
+    window.location.href = `/labs/host_collections/${data.id}`;
+  };
+
</code_context>

<issue_to_address>
**suggestion:** Directly setting window.location.href for navigation may bypass SPA routing.

Use the router's navigation method (such as history.push) to maintain SPA behavior and prevent unnecessary page reloads.

```suggestion
  import { useHistory } from 'react-router-dom';

  const [newName, setNewName] = useState(`Copy of ${hostCollection?.name || ''}`);
  const [copying, setCopying] = useState(false);

  const history = useHistory();

  const handleSuccess = (data) => {
    setCopying(false);
    dispatch(addToast({
      type: 'success',
      message: __('Host collection copied successfully'),
    }));
    onClose();
    history.push(`/labs/host_collections/${data.id}`);
  };
```
</issue_to_address>

### Comment 4
<location> `webpack/scenes/HostCollections/Details/DetailsTab/HostLimitEdit.js:64-66` </location>
<code_context>
+    setMaxHosts((maxHosts || 1) + 1);
+  };
+
+  const onChange = (event) => {
+    const value = Number(event.target.value);
+    setMaxHosts(value < 1 ? 1 : value);
</code_context>

<issue_to_address>
**suggestion:** NumberInput onChange handler may not handle empty input gracefully.

Clearing the input results in NaN, which prevents value updates. Please handle empty input explicitly or restrict clearing the field.
</issue_to_address>

### Comment 5
<location> `webpack/scenes/HostCollections/__tests__/HostCollectionsPage.test.js:22-31` </location>
<code_context>
+  [firstCollection] = results;
+});
+
+test('Can call API for Host Collections and show on screen on page load', async () => {
+  const autocompleteScope = nockInstance
+    .get(autocompleteUrl)
+    .query(true)
+    .reply(200, []);
+
+  const scope = nockInstance
+    .get(hostCollectionsPath)
+    .query(true)
+    .reply(200, hostCollectionsData);
+
+  const { queryByText } = renderWithRedux(
+    <HostCollectionsPage />,
+    renderOptions,
+  );
+
+  expect(queryByText(firstCollection.name)).toBeNull();
+
+  await patientlyWaitFor(() => {
+    expect(queryByText(firstCollection.name)).toBeInTheDocument();
+  });
+
+  assertNockRequest(autocompleteScope);
+  assertNockRequest(scope);
+});
</code_context>

<issue_to_address>
**suggestion (testing):** Missing tests for error states and API failures.

Add tests for API error responses (e.g., 500, 404) to confirm error handling and toast notifications work as expected.

Suggested implementation:

```javascript
test('Can call API for Host Collections and show on screen on page load', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, []);

  const scope = nockInstance
    .get(hostCollectionsPath)
    .query(true)
    .reply(200, hostCollectionsData);

  const { queryByText } = renderWithRedux(
    <HostCollectionsPage />,
    renderOptions,
  );

  expect(queryByText(firstCollection.name)).toBeNull();

  await patientlyWaitFor(() => {
    expect(queryByText(firstCollection.name)).toBeInTheDocument();
  });

  assertNockRequest(autocompleteScope);
  assertNockRequest(scope);
});

test('Shows error message when Host Collections API returns 500', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, []);

  const scope = nockInstance
    .get(hostCollectionsPath)
    .query(true)
    .reply(500, { message: 'Internal Server Error' });

  const { getByText } = renderWithRedux(
    <HostCollectionsPage />,
    renderOptions,
  );

  await patientlyWaitFor(() => {
    expect(getByText(/error/i)).toBeInTheDocument();
  });

  assertNockRequest(autocompleteScope);
  assertNockRequest(scope);
});

test('Shows error message when Host Collections API returns 404', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(200, []);

  const scope = nockInstance
    .get(hostCollectionsPath)
    .query(true)
    .reply(404, { message: 'Not Found' });

  const { getByText } = renderWithRedux(
    <HostCollectionsPage />,
    renderOptions,
  );

  await patientlyWaitFor(() => {
    expect(getByText(/error/i)).toBeInTheDocument();
  });

  assertNockRequest(autocompleteScope);
  assertNockRequest(scope);
});

test('Shows error message when autocomplete API returns 500', async () => {
  const autocompleteScope = nockInstance
    .get(autocompleteUrl)
    .query(true)
    .reply(500, { message: 'Internal Server Error' });

  const scope = nockInstance
    .get(hostCollectionsPath)
    .query(true)
    .reply(200, hostCollectionsData);

  const { getByText } = renderWithRedux(
    <HostCollectionsPage />,
    renderOptions,
  );

  await patientlyWaitFor(() => {
    expect(getByText(/error/i)).toBeInTheDocument();
  });

  assertNockRequest(autocompleteScope);
  assertNockRequest(scope);
});

```

- If your error UI uses a specific text (e.g., "An error occurred" or a toast notification), replace `/error/i` in `getByText` with the actual error message or selector.
- If you use a custom toast notification system, you may need to use `queryByRole('alert')` or similar queries to check for the toast.
- Ensure that your error handling in `HostCollectionsPage` actually renders an error message when the API fails.
</issue_to_address>

---

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

• X
• Mastodon
• LinkedIn
• Facebook

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

[sourcery-ai]

suggestion: The restrictedSearchQuery function may not handle complex user search queries robustly.

Prepending the filter may cause issues with queries containing logical operators or parentheses. Consider wrapping the user input in parentheses or using a more reliable method to combine filters.

Suggested change

	// Restrict search to only hosts in this collection
	const restrictedSearchQuery = (userSearch) => {
	const filter = `host_collection_id=${hostCollectionId}`;
	const trimmedSearch = userSearch?.trim() ?? '';
	if (!!trimmedSearch && !trimmedSearch.includes(filter)) {
	return `${filter} and ${trimmedSearch}`;
	}
	return filter;
	};
	// Restrict search to only hosts in this collection
	const restrictedSearchQuery = (userSearch) => {
	const filter = `host_collection_id=${hostCollectionId}`;
	const trimmedSearch = userSearch?.trim() ?? '';
	if (!!trimmedSearch && !trimmedSearch.includes(filter)) {
	// Wrap user search in parentheses to preserve logical grouping
	return `${filter} and (${trimmedSearch})`;
	}
	return filter;
	};

[sourcery-ai]

suggestion: Directly setting window.location.href for navigation may bypass SPA routing.

Use the router's navigation method (such as history.push) to maintain SPA behavior and prevent unnecessary page reloads.

Suggested change

	const [newName, setNewName] = useState(`Copy of ${hostCollection?.name || ''}`);
	const [copying, setCopying] = useState(false);
	const handleSuccess = (data) => {
	setCopying(false);
	dispatch(addToast({
	type: 'success',
	message: __('Host collection copied successfully'),
	}));
	onClose();
	window.location.href = `/labs/host_collections/${data.id}`;
	};
	import { useHistory } from 'react-router-dom';
	const [newName, setNewName] = useState(`Copy of ${hostCollection?.name || ''}`);
	const [copying, setCopying] = useState(false);
	const history = useHistory();
	const handleSuccess = (data) => {
	setCopying(false);
	dispatch(addToast({
	type: 'success',
	message: __('Host collection copied successfully'),
	}));
	onClose();
	history.push(`/labs/host_collections/${data.id}`);
	};

[jeremylenz]

Haven't spun it up yet, but here are some initial comments to pass on to Claude :)

[jeremylenz]

Katello already has an ActionableDetail component (used in ContentViewInfo) that does this same thing. Make sure to tell your AI to inspect the Katello code base and follow existing patterns - it likes to default to reinventing the wheel.

[jeremylenz]

This seems like wheel-reinvention too. Tell it to look in webpack/scenes/Tasks/helpers.js and reuse stuff from there.

[jeremylenz]

I feel like Patternfly's NumberInput does a lot of this logic for you -- https://github.com/katello/katello/blob/0832f953654ae28b723f6108f3f09d7f47373148/webpack/scenes/ActivationKeys/Details/components/EditModal.js#L142-L143

[jeremylenz]

DON'T SAY SUCCESSFULLY 😠

[jeremylenz]

Same comment as above - reuse the toast helpers already in Katello.

[jeremylenz]

Tell your AI about propsToCamelCase :)

[jeremylenz]

Suggested change

	successToast: () => __('Host collection updated successfully'),
	successToast: () => __('Host collection updated'),

[jeremylenz]

Suggested change

	title: __('Content Hosts'),
	title: __('Hosts'),

We don't use the term "Content hosts" anymore.

[jeremylenz]

This may be the one place where title case was correct.

Suggested change

	header={__('Host collections')}
	header={__('Host Collections')}

[jeremylenz]

Why have a reducer if it's not going to alter the state?

Actually, tell your AI to look at useAPI and see if we can use that instead. I haven't needed to add a reducer to Katello in ages.

[sjha4]

I have an agent for angular to react conversions I iterated upon a little using Sync plans UI..
https://gist.github.com/sjha4/66c62efc4df6b159198bb4ba17a8ad24

I am wondering if we can also try this agent for hostcollections and compare the results.

[sjha4]

Review by angular-to-react-migrator Agent

I've completed a comprehensive review of this AngularJS to React migration. The PR demonstrates excellent UI implementation and use of modern React patterns, but there are critical architectural issues that need to be addressed to align with established Katello/Foreman migration patterns.

---

🎯 Key Findings Summary

✅ Excellent Implementation Areas

1. Outstanding TableIndexPage Usage - Proper column configuration, kebab actions, and permission-based visibility
2. Excellent Bulk Selection - Exemplary use of useBulkSelect and SelectAllCheckbox in both HostsTab and AddHostsModal
3. Well-Designed InlineEdit Component - Reusable, accessible, with keyboard navigation
4. Smart Hash-Based Tab Navigation - Enables deep linking to specific tabs
5. Proper Permission Handling - Conditional action visibility based on user permissions
6. Restricted Search Pattern - Smart filtering in HostsTab to scope results

❌ Critical Issues (Must Fix Before Merge)

1. Custom Thunk Actions Instead of Foreman API Helpers (HostCollectionsActions.js)

   • Uses custom async thunks with manual try/catch blocks
   • Should use post(), del() from foremanReact/redux/API like HostCollectionDetailsActions.js does
   • Violates established Katello migration pattern

2. Unnecessary Custom Reducer (HostCollectionsReducer.js)

   • Empty reducer that does nothing
   • Should be deleted - migration pattern uses generic API Redux system
   • Remove from webpack/redux/reducers/index.js

3. Custom Selectors Accessing Wrong State Path (HostCollectionsSelectors.js)

   • Should use selectAPIResponse, selectAPIStatus, selectAPIError from Foreman
   • Current selectors access state[HOST_COLLECTIONS_KEY] which doesn't exist with API reducer

4. Inconsistent Action Patterns

   • Details actions correctly use Foreman API helpers ✅
   • Main actions use custom thunks ❌
   • These should be consistent

⚠️ Minor Issues

5. Full page reload after create instead of navigation to details page
6. Consider adding organization_id to API calls (verify if backend requires it)

---

📊 Architecture Comparison

Pattern	Reference (FlatpakRemotes)	This PR	Status
TableIndexPage	✅	✅	Good
Foreman API helpers	✅	❌	Needs Fix
API selectors	✅	❌	Needs Fix
No custom reducer	✅	❌	Needs Fix
Bulk selection	N/A	✅	Excellent
Permission handling	✅	✅	Good

---

🔧 Required Changes

See inline comments for specific code examples. The main architectural changes needed:

1. Refactor HostCollectionsActions.js to use Foreman's post() and del() helpers
2. Delete HostCollectionsReducer.js and remove from Redux registration
3. Rewrite HostCollectionsSelectors.js to use Foreman's API selectors
4. Update constants to include separate keys for create/copy/delete operations

---

📚 Reference Implementation

Your HostCollectionDetailsActions.js file shows the correct pattern:

export const updateHostCollection = (id, params, handleSuccess) => put({
  type: API_OPERATIONS.PUT,
  key: `UPDATE_HOST_COLLECTION_${id}`,
  url: katelloApi.getApiUrl(`/host_collections/${id}`),
  params,
  successToast: () => __('Host collection updated successfully'),
  errorToast: error => error.response?.data?.error?.message,
  handleSuccess,
});

Apply this same pattern to the create/copy/delete actions in HostCollectionsActions.js.

---

🎓 Final Verdict

Overall Quality: Good implementation with critical architecture issues
Recommendation: Request changes to align with Katello migration patterns

The functionality is solid and the UI implementation is excellent, but the architecture needs to match established conventions (like FlatpakRemotes, ContentViews, etc.) for maintainability and consistency across the codebase.

Once the architectural issues are resolved, this will be a great addition to the React migration effort! 🚀

[sjha4]

❌ CRITICAL: Custom Thunk Actions Instead of Foreman API Helpers

This file uses custom async thunk actions with manual try/catch blocks instead of Foreman's standardized API helpers. This violates the established Katello migration pattern.

Should use this pattern instead (like FlatpakRemotes and the Details actions):

import { API_OPERATIONS, post, del } from 'foremanReact/redux/API';
import api, { orgId } from '../../services/api';

export const createHostCollection = params => post({
  type: API_OPERATIONS.POST,
  key: CREATE_HOST_COLLECTION_KEY,
  url: api.getApiUrl('/host_collections'),
  params: { organization_id: orgId(), ...params },
  successToast: () => __('Host collection created successfully'),
  errorToast: error => error.response?.data?.error?.message || __('Failed to create host collection'),
});

Why this matters:

• Foreman's API helpers provide consistent error handling, toast notifications, and Redux state management
• The generic API Redux system is used across all Foreman/Katello features
• Your HostCollectionDetailsActions.js already uses this pattern correctly - this file should match that approach

[sjha4]

❌ CRITICAL: Unnecessary Custom Reducer

This empty reducer should be deleted. The migration pattern explicitly states: "NO custom reducers - Use generic API Redux system instead"

• This reducer does nothing (just returns state on all actions)
• All API state is managed by Foreman's generic API reducer using selectors
• Other migrated features (FlatpakRemotes, ContentViews) don't have custom reducers

Fix:

1. Delete this file
2. Remove the import from webpack/redux/reducers/index.js
3. Use API selectors with selectAPIResponse(state, key) instead

[sjha4]

❌ CRITICAL: Custom Selectors Accessing Wrong State Path

This selector tries to access state[HOST_COLLECTIONS_KEY] directly, which doesn't exist when using Foreman's API reducer.

Should use Foreman's API selectors instead:

import {
  selectAPIStatus,
  selectAPIError,
  selectAPIResponse,
} from 'foremanReact/redux/API/APISelectors';
import { HOST_COLLECTIONS_KEY } from './HostCollectionsConstants';

export const selectHostCollections = (state, index = '') =>
  selectAPIResponse(state, HOST_COLLECTIONS_KEY + index) || {};

export const selectHostCollectionsStatus = (state, index = '') =>
  selectAPIStatus(state, HOST_COLLECTIONS_KEY + index);

export const selectHostCollectionsError = (state, index = '') =>
  selectAPIError(state, HOST_COLLECTIONS_KEY + index);

Reference: See how FlatpakRemotes does this correctly.

[sjha4]

✅ EXCELLENT: Bulk Selection Implementation

This is an outstanding example of using useBulkSelect and SelectAllCheckbox! The implementation correctly:

• Handles both individual selections and select-all mode
• Provides proper toolbar integration
• Works with paginated data
• Extracts selected IDs correctly for both modes

This pattern should be used as a reference for other bulk selection implementations.

[sjha4]

⚠️ MINOR: Full Page Reload Instead of Navigation

Using window.location.reload() forces a full page reload. Better to navigate to the new host collection's details page:

const handleSuccess = (data) => {
  setSaving(false);
  dispatch(addToast({
    type: 'success',
    message: __('Host collection created successfully'),
  }));
  onClose();
  // Navigate to details page instead of reloading
  window.location.href = `/labs/host_collections/${data.id}`;
};

This matches the pattern used in CopyHostCollectionModal.js.

[jeremylenz]

I really like Sourcery's "Prompt for AI Agents" that I can paste into Claude. @sjha4 Can you have your agent output its comments in a similar way so it's more easily transferable?