Fixes #39408 - Move FieldConstructor from webhooks plugin to core

[Lukshio]

Move FieldConstrutor from webhooks plugin to core so it can be reused across foreman

[ofedoren]

Code Review (automated — Claude)

1. XSS vulnerability — dangerouslySetInnerHTML (High)

The FieldConstructor component renders labelHelp as raw HTML:

<div dangerouslySetInnerHTML={{ __html: labelHelp }} />

This was already present in the webhooks plugin, but moving it to core significantly increases its exposure surface — any plugin or feature using this shared component could pass unsanitized user input as labelHelp. If this component is going to be reused broadly, this should either be sanitized (e.g. via DOMPurify) or switched to rendering text/JSX only. At minimum, this should be called out as an existing risk being promoted.

2. Missing index.js barrel export

Core components typically have an index.js re-export file (e.g. AutocompleteInput/index.js). This PR doesn't add one, so consumers will need to import the full path FieldConstructor/FieldConstructor. Adding an index.js with export { default } from './FieldConstructor' would be consistent with the rest of the codebase.

3. defaultProps is deprecated

React has deprecated defaultProps for function components (removed in React 19). Since this is a fresh addition to core, it's a good opportunity to migrate to JavaScript default parameters in the destructuring. Not a blocker, but worth doing now rather than accumulating tech debt.

4. /* eslint-disable max-lines */ at the top

Suppressing linting for the whole file is a code smell. If the file is too long, consider splitting FormField and ValidationComponent into separate modules. If keeping as-is, at least use a more targeted eslint-disable-next-line or raise the threshold in .eslintrc.

5. Test quality

The tests are a direct copy from the webhooks plugin. A few observations:

• The "Autocomplete render test" description has a double space (render test)
• Tests use document.getElementById directly instead of RTL queries (getByRole, getByLabelText) — not idiomatic RTL
• No test coverage for the dangerouslySetInnerHTML path in labelHelp
• No test for the password field with setIsPasswordDisabled toggle

Minor nits

• FormField accepts ...props in destructuring but never uses them — the spread is silently swallowed
• type is passed as a prop to Checkbox — PF4/PF5 Checkbox doesn't use a type prop, this is a no-op

---

Note: This review was generated by Claude.

[ofedoren]

Since we're at it, I'd say let's address the points noted by Claude.

[Lukshio]

Code Review (automated — Claude)

@ofedoren

1. Fixed
2. We are not doing it anymore

   foreman/developer_docs/pf3-to-pf5-migration-guide.asciidoc

   Line 27 in b58f67d

   * Prefer *direct imports* from the concrete module (e.g. the component file) over importing through a folder `index.js` when that avoids an extra hop, ambiguous re-exports, and inconsistent naming across the codebase.

3. We are at v16.9
4. Exceeds lines by 11 rows -> Lot of imports and prop value (210 rows of code), it's ok to have disabled max lines in this case
5. tests fixed
6. nitpick adressed

[ofedoren]

Re-review after update (automated — Claude)

The major issues from the first review are all resolved — XSS via dangerouslySetInnerHTML removed, ...props properly forwarded, password toggle tested, stray type prop on Checkbox removed. The update looks clean.

A few observations on pre-existing behavior (carried over from the webhooks plugin) that are worth being aware of now that this is a shared core component:

1. isLoading only partially wired — It's used in the autocomplete branch (isDisabled={disabled || isLoading}) but AutocompleteInput doesn't itself accept isLoading. For all other field types (text, textarea, checkbox), isLoading is destructured but has no effect. Consumers might expect loading state on non-autocomplete fields.

2. validated prop creates sticky errors — When a parent passes validated='error', the internal fieldValidated state gets set to 'error'. But clearing it requires the user to change the value AND the parent to stop passing validated='error' simultaneously. The parent can't unilaterally clear the error state. This may surprise consumers of the shared component.

Neither of these are blockers — they're inherited behavior. Just flagging for awareness since the component now has broader scope.

Note: This review was generated by Claude.

[ofedoren]

Seems it's mergable now, I need to test it with webhooks first anyway.

Regarding new Claude comment: it might be worth taking a look at it right now (in a separate commit though), but I have a feeling that it's related to #10983 and we need to have that within the context as well and check if it still works.