Fixes #39200 - Migrate Model Pages to React by pondrejk · Pull Request #10943 · theforeman/foreman

pondrejk · 2026-03-31T14:19:25Z

Converting HW models pages, I did this for learning purposes looking at some more recent rewrites elsewhere in the project, so don't judge me too hard. In a way a follow up to #10910 that removed the PF3 button used in models.

Tests are AI assisted

adamruzicka · 2026-04-15T13:12:57Z

+          name="model_hardware_model"
+          type="text"
+          ouiaId="model_hardware_model-input"
+          required


L54 says only name is required

adamruzicka · 2026-04-15T13:13:05Z

+          name="model_vendor_class"
+          type="text"
+          ouiaId="model_vendor_class-input"
+          required


L54 says only name is required

Copilot

Pull request overview

Migrates the Hardware Models UI pages (index/new/edit) from Rails views to React routes, aligning Models with other recently migrated SPA pages in the Foreman UI.

Changes:

Expand the React Models route export to include /models/new and /models/:id/edit, and wire it into the main route list.
Add new React pages/components for creating and editing models, including a shared ModelForm and associated JS tests.
Update Rails routing to serve React for models#new and models#edit, remove legacy ERB templates, and adjust Ruby/JS test coverage accordingly.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
webpack/assets/javascripts/react_app/routes/routes.js	Spreads the `Models` route list into the main React routes array.
webpack/assets/javascripts/react_app/routes/test/snapshots/Routes.test.js.snap	Updates snapshot to include new React routes for models new/edit.
webpack/assets/javascripts/react_app/routes/Models/index.js	Changes Models route export from a single route to an array (index/new/edit).
webpack/assets/javascripts/react_app/routes/Models/constants.js	Adds constants for new/edit model paths.
webpack/assets/javascripts/react_app/routes/Models/NewModelFormPage.js	New React page for creating a model via API submit + breadcrumb.
webpack/assets/javascripts/react_app/routes/Models/ModelsPage/index.js	Uses React Router navigation for create and edit links.
webpack/assets/javascripts/react_app/routes/Models/ModelForm.test.js	Adds unit tests for the shared React model form.
webpack/assets/javascripts/react_app/routes/Models/ModelForm.js	Introduces shared PF4 form component for new/edit model.
webpack/assets/javascripts/react_app/routes/Models/EditModelFormPage.js	New React page for editing a model (loads initial values via API).
webpack/assets/javascripts/react_app/redux/actions/common/forms.js	Adjusts default success toast to distinguish create vs update.
test/integration/model_js_test.rb	Adds JS integration test coverage for deleting a model from the index.
test/controllers/models_controller_test.rb	Removes controller tests tied to removed `new/edit` templates.
test/controllers/api/v2/models_controller_test.rb	Adds API tests ensuring blank names are rejected on create/update.
config/routes.rb	Routes models `new/edit` to React and keeps `models` index handled by React.
app/views/models/new.html.erb	Removes legacy Rails view for model creation page.
app/views/models/edit.html.erb	Removes legacy Rails view for model edit page.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T13:25:19Z

  resources :models, except: [:show, :index] do
+    member do
+      get 'edit', to: 'react#index'
+    end
    collection do
+      get 'new', to: 'react#index', as: 'new'
      get 'auto_complete_search'
    end
  end


resources :models, except: [:show, :index] still generates the default new/edit routes to ModelsController#new/#edit, but the corresponding templates were removed in this PR. This can lead to route conflicts (depending on route order) and/or MissingTemplate errors when hitting /models/new or /models/:id/edit. Align this with the filters migration pattern by excluding :new and :edit (and relying on the explicit react#index routes), or otherwise ensure the default routes are not reachable.

Copilot · 2026-04-15T13:25:19Z

+          name="model_hardware_model"
+          type="text"
+          ouiaId="model_hardware_model-input"
+          required


hardware_model is marked required in the input, but the backend model only validates presence for name (hardware_model can be blank). With required set, the browser will block form submission unless this field is filled, changing behavior from the previous form.

Suggested change

required

Copilot · 2026-04-15T13:25:19Z

+          name="model_vendor_class"
+          type="text"
+          ouiaId="model_vendor_class-input"
+          required


vendor_class is also marked required, but the backend does not require it. This will prevent submitting the form with vendor_class left blank, which differs from the previous Rails form and model validations.

Suggested change

required

Copilot · 2026-04-15T13:25:20Z

+      <FormGroup label={__('Name')} isRequired>
+        <TextInput
+          id="model_name"
+          name="model_name"
+          type="text"
+          ouiaId="model_name-input"
+          required
+          value={values.name}
+          onChange={handleChange('name')}
+        />


initialValues can contain undefined values (your tests cover name: undefined), but the inputs pass value={values.name} etc. Passing undefined as an input value causes uncontrolled/controlled warnings and can break PatternFly inputs. Consider normalizing all field values to '' when storing state or when passing to TextInput/TextArea (e.g., value={values.name || ''}).

the test passing undefined for a name checks the negative scenario, asserts such value cannot be submitted

Copilot · 2026-04-15T13:25:20Z

+  useEffect(() => {
+    if (!response) return;
+
+    setValues(prevValues => ({
+      ...prevValues,
+      name: response.name || '',
+      hardware_model: response.hardware_model || '',
+      vendor_class: response.vendor_class || '',
+      info: response.info || '',
+    }));


The useEffect that hydrates values from response will overwrite any user edits made before the API call finishes (slow network, user starts typing quickly), causing lost input. Consider rendering the form only after the initial response is loaded, or only initializing state once (e.g., track an isLoaded/isDirty flag) so late responses don't clobber user changes.

Copilot · 2026-04-15T13:25:20Z

+          ? '%s was successfully updated.'
+          : '%s was successfully created.',


defaultSuccessToast constructs new user-facing strings ("%s was successfully updated." / "%s was successfully created.") without marking them for translation. Since this code is shared across the app and other messages in this module are wrapped in __(), these should also be translatable (e.g., wrap the full template strings in __).

Suggested change

? '%s was successfully updated.'

: '%s was successfully created.',

? __('%s was successfully updated.')

: __('%s was successfully created.'),

Lukshio

Hi, thanks for the PR, code is clean, tests are looking good. Just some minor changes below.

Lukshio · 2026-04-20T23:10:51Z

+
+  const initialValues = useMemo(
+    () => ({
+      name: response.name || '',


This should be probably always set to response.name since it cannot be submitted without name. Or replaced with state in combination with sucessHandler from API call

Lukshio · 2026-04-20T23:12:03Z

+        isLoading
+        breadcrumbOptions={breadcrumbOptions}
+      >
+        {null}


PageLayout children cannot be null

Lukshio · 2026-04-20T23:21:31Z

+}) => {
+  const dispatch = useDispatch();
+  const history = useHistory();
+  const { response, status } = useAPI('get', `${MODELS_API_PATH}/${id}`);


I would prefer to use APIActions.get() and set the values as a successHandler, instead of catching it by useMemo()

Lukshio · 2026-04-20T23:24:39Z

+  useEffect(() => {
+    setValues(initialValues);
+  }, [initialValues]);


Initial values should be set only once. Maybe I am missing something, but why it's set once in useState and then set once more here?

pondrejk · 2026-04-21T13:29:01Z

@Lukshio @adamruzicka hi, I think I addressed all the comments, can you please revisit?

Lukshio · 2026-04-21T17:01:06Z

-    [response]
-  );
+  useEffect(() => {
+    setInitialValues(null);


Suggested change

setInitialValues(null);

This is not needed, state has already null as initial value - L31

Lukshio · 2026-04-21T17:02:09Z

+        },
+      })
+    );
+  }, [dispatch, id]);


Suggested change

}, [dispatch, id]);

// eslint-disable-next-line react-hooks/exhaustive-deps

}, []);

Empty dependency array will run the effect only once when page will be mounted, that's what we need.

I had a problem with this when switching models trough the breadcrumb menu, the form didn't re-render after that

Lukshio · 2026-04-21T17:09:20Z

PageLayout should be used only once, but the child should be rendered conditionally.

Lukshio · 2026-04-22T16:21:33Z

The loading spinner is present on loaded page. Loading could be also implemented by showing form but with disabled fields, until loading finish.

adamruzicka · 2026-04-24T13:57:24Z

Trying to submit a model which would cause a name collision just shows a toast saying Oh no! Something went wrong while submitting the form, the server returned the following error: Name has already been taken.

The actual api response is this

{
  "error": {"id":null,"errors":{"name":["has already been taken"]},"full_messages":["Name has already been taken"]}
}

Since we have all the data (the field and the actual error), could we show that in-line?

adamruzicka · 2026-04-27T08:39:22Z

+    state => selectAPIResponse(state, modelNamesFetchKey),
+    shallowEqual
+  );
+  const existingNames =


As in, preload up to 10000 hw model names to do name conflict checking on the frontend? That's potentially a lot of data to load and it can't guarantee conflicts won't arise. Couldn't we just display an inline error in case form submit fails?

good point, but would it still be inline check if triggered only on submit? AI friend suggested some debounce search, wdyt?

Debounce search sounds like a good solution. Just disable the submit button until the search is finished. It would also be nice to have a loading indicator to show that something is happening.

but would it still be inline check if triggered only on submit?

Originally by inline I meant "error shown close to the field that caused it", not necessarily "checks are happening as you type". I'd be fine with sticking with on submit validations.

Debounce search sounds like a good solution.

Do we have this pattern anywhere already? By replicating the validations on the frontend, we'd be losing the "backend is the one and only source of truth" stance

I don't think we have something like this anywhere, but there are different inline validations that work as you type. So I guess the question is more if we want to start doing this.

I see this more as a convenience for users, so they don't have to find out about the problem only after submitting, the backend validation is still there on guard.

I'm also ok with going without this, but in such case I'd just keep the toast. Showing server error inline could imho be confusing, someone could think it is a buggy "as you type" check that didn't fire on time

I don't think we have something like this anywhere, but there are different inline validations that work as you type. So I guess the question is more if we want to start doing this.

Eventually yes, but I'd say this should be approached systematically.

I'm also ok with going without this, but in such case I'd just keep the toast. Showing server error inline could imho be confusing, someone could think it is a buggy "as you type" check that didn't fire on time

Well, the old page just reset so anything is an improvement over that, as long as the error has some details and not just "oh no, it's on fire :^)"

With that being said, let's postpone it for later.

Lukshio · 2026-04-29T13:50:34Z

+          id="model_info"
+          name="model_info"
+          rows={7}
+          ouiaId="info-textarea"


ouiaId prop does not exists for TextArea component

Lukshio · 2026-04-29T13:54:34Z

+    <Stack hasGutter>
+      <StackItem>
+        {labelSkeleton}
+        <Skeleton height="36px" screenReaderText={__('Loading form')} />


Suggested change

<Skeleton height="36px" screenReaderText={__('Loading form')} />

<Skeleton height="36px" screenreaderText={__('Loading form')} />

Fix typo

Lukshio · 2026-05-04T15:12:34Z

+const labelSkeleton = <Skeleton height="14px" width="30%" />;
+
+const ModelFormSkeleton = () => (
+  <Form isWidthLimited ouiaId="model-form-skeleton">


Suggested change

<Form isWidthLimited ouiaId="model-form-skeleton">

<Form isWidthLimited id="model-form-skeleton">

I missed this one, Form does not have ouiaId

Lukshio · 2026-05-05T10:26:06Z

Changes looks good. Dev + QE ack.

Associated PRs:

#10974
SatelliteQE/robottelo#21166
SatelliteQE/airgun#2390

pondrejk · 2026-05-06T08:32:43Z

squashed after ack

adamruzicka · 2026-05-07T07:32:00Z

Thank you @pondrejk & @Lukshio !

pr-processor Bot added the Not yet reviewed label Mar 31, 2026

github-actions Bot added the UI label Mar 31, 2026

This was referenced Apr 1, 2026

update hardware model view for UI migration SatelliteQE/airgun#2390

Open

hw model react migration tests SatelliteQE/robottelo#21166

Draft

pondrejk force-pushed the reactmodels branch 3 times, most recently from 04b2c2b to 22ce3f8 Compare April 1, 2026 14:09

pr-processor Bot added the Waiting on contributor label Apr 15, 2026

pondrejk force-pushed the reactmodels branch from 6531031 to 5c326c6 Compare April 15, 2026 11:27

pr-processor Bot removed the Waiting on contributor label Apr 15, 2026

adamruzicka requested changes Apr 15, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Not yet reviewed labels Apr 15, 2026

adamruzicka requested a review from Copilot April 15, 2026 13:19

Copilot started reviewing on behalf of adamruzicka April 15, 2026 13:19 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Apr 17, 2026

pondrejk force-pushed the reactmodels branch 3 times, most recently from eaafbc2 to 3af6799 Compare April 20, 2026 07:03

Lukshio suggested changes Apr 20, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor Needs re-review and removed Needs re-review Waiting on contributor labels Apr 20, 2026

Lukshio suggested changes Apr 21, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Needs re-review labels Apr 21, 2026

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Apr 22, 2026

pr-processor Bot removed the Needs re-review label Apr 24, 2026

pondrejk force-pushed the reactmodels branch from 55639d8 to 038efe3 Compare April 24, 2026 10:04

adamruzicka reviewed Apr 27, 2026

View reviewed changes

Lukshio suggested changes Apr 29, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor Needs re-review and removed Waiting on contributor labels Apr 29, 2026

Lukshio suggested changes May 4, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Needs re-review labels May 4, 2026

pondrejk force-pushed the reactmodels branch from ec1286b to de093ff Compare May 5, 2026 07:27

pr-processor Bot added Needs re-review and removed Waiting on contributor labels May 5, 2026

Fixes #39200 - Migrate Model Pages to React

558c13b

pondrejk force-pushed the reactmodels branch from de093ff to 558c13b Compare May 6, 2026 07:12

adamruzicka approved these changes May 7, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label May 7, 2026

adamruzicka merged commit 8af49dd into theforeman:develop May 7, 2026
26 checks passed

		? '%s was successfully updated.'
		: '%s was successfully created.',

	}, [dispatch, id]);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

	<Skeleton height="36px" screenReaderText={__('Loading form')} />
	<Skeleton height="36px" screenreaderText={__('Loading form')} />

	<Form isWidthLimited ouiaId="model-form-skeleton">
	<Form isWidthLimited id="model-form-skeleton">

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Lukshio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pondrejk commented Apr 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukshio commented Apr 22, 2026

Uh oh!

adamruzicka commented Apr 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pondrejk Apr 27, 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 Apr 27, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pondrejk commented Mar 31, 2026 •

edited

Loading

pondrejk Apr 27, 2026 •

edited

Loading

Lukshio Apr 27, 2026 •

edited

Loading