Add useFormValidation hook and migrate the user forms onto it - #50435
Add useFormValidation hook and migrate the user forms onto it#50435nulmete wants to merge 2 commits into
Conversation
Adds frontend/hooks/useFormValidation.ts as the single source of truth for the form validation behavior documented in frontend/docs/patterns.md, and migrates UserForm and ApiUserForm onto it as the reference implementations.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50435 +/- ##
==========================================
+ Coverage 68.20% 68.22% +0.01%
==========================================
Files 3943 3948 +5
Lines 251273 251465 +192
Branches 13406 13302 -104
==========================================
+ Hits 171390 171562 +172
- Misses 64532 64552 +20
Partials 15351 15351
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WalkthroughThe PR adds the shared Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 2
🧹 Nitpick comments (2)
frontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsx (1)
139-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify
onAccessTypeChangeand confirm the intended endpoint-selection behavior.On the
isSpecific === truebranch,api_endpoints: formData.api_endpointsre-assigns the current value. It adds no change but marksapi_endpointsdirty. It also readsformDatafrom the render closure rather than the hook's internal current value.A user who selects "All API endpoints" and then returns to "Specific API endpoints" loses the previous endpoint list, because the first change sets it to
[]. Confirm that this reset is intended.♻️ Proposed simplification
const onAccessTypeChange = (isSpecific: boolean) => { - commitFields({ - isSpecificEndpoints: isSpecific, - api_endpoints: isSpecific ? formData.api_endpoints : [], - }); + commitFields( + isSpecific + ? { isSpecificEndpoints: true } + : { isSpecificEndpoints: false, api_endpoints: [] } + ); };🤖 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 `@frontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsx` around lines 139 - 144, Update onAccessTypeChange to avoid reassigning api_endpoints when isSpecific is true, and use the hook’s current field value rather than the render-closure formData where needed. Preserve the existing reset-to-empty behavior for the false branch only; confirm that switching back to specific endpoints should not restore the previously cleared selections.frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx (1)
672-703: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUse a per-instance id for the submit-form association.
FORM_IDis module-scoped, so multipleUserForminstances would render duplicate form ids. The modal submit button passes this throughButton.formIdasbutton.form, which can target the first matching form in this edge case. DeriveformIdper instance withuseIdand use it in both the<form id=...>and footer submit button.🤖 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 `@frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx` around lines 672 - 703, The module-scoped FORM_ID causes duplicate form identifiers across UserForm instances. In the UserForm component, derive a per-instance formId with useId and replace FORM_ID in both the form id attribute and the footer submit Button’s formId prop, preserving the existing submit behavior.
🤖 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 `@frontend/hooks/useFormValidation.ts`:
- Around line 122-123: Implement the form-level isValid contract in
frontend/hooks/useFormValidation.ts: add isValid to the returned API without
using it to disable submission, recompute it after setField and commitFields
update form data, and recompute it during reset. Add coverage in
frontend/hooks/useFormValidation.tests.ts for invalid initial data,
valid/invalid transitions for text fields and committed controls, and reset
behavior.
- Around line 174-176: Remove the render-time assignments to validateRef,
skipTrimRef, and isSubmittingRef in the useFormValidation hook. Update these
refs from an effect or other non-render path, or refactor the stable callbacks
to avoid mutable latest-value refs while preserving access to the current
validator, trim setting, and submitting state.
---
Nitpick comments:
In `@frontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsx`:
- Around line 139-144: Update onAccessTypeChange to avoid reassigning
api_endpoints when isSpecific is true, and use the hook’s current field value
rather than the render-closure formData where needed. Preserve the existing
reset-to-empty behavior for the false branch only; confirm that switching back
to specific endpoints should not restore the previously cleared selections.
In `@frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx`:
- Around line 672-703: The module-scoped FORM_ID causes duplicate form
identifiers across UserForm instances. In the UserForm component, derive a
per-instance formId with useId and replace FORM_ID in both the form id attribute
and the footer submit Button’s formId prop, preserving the existing submit
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d1368eb-1ca2-4e7c-a79c-83d34df78d5b
⛔ Files ignored due to path filters (2)
.claude/rules/fleet-frontend.mdis excluded by!**/*.mdfrontend/docs/patterns.mdis excluded by!**/*.md
📒 Files selected for processing (13)
changes/use-form-validation-hookfrontend/components/buttons/Button/Button.tsxfrontend/hooks/useFormValidation.tests.tsfrontend/hooks/useFormValidation.tsfrontend/interfaces/user.tsfrontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsxfrontend/pages/admin/ManageUsersPage/CreateUserPage/CreateUserPage.tsxfrontend/pages/admin/ManageUsersPage/EditUserPage/EditUserPage.tsxfrontend/pages/admin/ManageUsersPage/components/AddUserModal/AddUserModal.tsxfrontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsxfrontend/pages/admin/ManageUsersPage/components/EditUserModal/EditUserModal.tsxfrontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tests.tsxfrontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx
💤 Files with no reviewable changes (1)
- frontend/interfaces/user.ts
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR introduces a shared useFormValidation hook to standardize Fleet’s form validation behavior (error timing, clearing rules, submit checkpoint behavior, trimming, and in-flight guarding) and migrates the user-management forms to use it, along with updated docs and a small Button enhancement to support submit buttons rendered outside a <form>.
Changes:
- Added
frontend/hooks/useFormValidation.tswith unit tests, and documented it infrontend/docs/patterns.mdand.claude/rules/fleet-frontend.md. - Migrated
UserFormandApiUserFormto the hook, including inline “select at least one fleet” errors and updated validation copy. - Updated user create/edit pages/modals to pass field-level API errors as
serverErrors, and addedButtonsupport for submitting an external form viaformattribute.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/admin/ManageUsersPage/EditUserPage/EditUserPage.tsx | Switches page-level error state to IFormErrors, updates server error copy, and passes serverErrors into UserForm. |
| frontend/pages/admin/ManageUsersPage/CreateUserPage/CreateUserPage.tsx | Adds getFieldErrors helper for mapping API failures to inline errors; switches to IFormErrors and passes serverErrors. |
| frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx | Refactors the core user add/edit form to useFormValidation, adds inline fleets error, and moves submission to <form onSubmit>. |
| frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tests.tsx | Updates/extends tests for new validation behavior and copy; adds new test cases for submit/inline fleets error. |
| frontend/pages/admin/ManageUsersPage/components/EditUserModal/EditUserModal.tsx | Updates prop types and passes serverErrors into UserForm. |
| frontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsx | Refactors API-only user form to useFormValidation, consolidating validation and submit flow. |
| frontend/pages/admin/ManageUsersPage/components/AddUserModal/AddUserModal.tsx | Updates prop types and passes serverErrors into UserForm. |
| frontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx | Updates local state types from IUserFormErrors to IFormErrors. |
| frontend/interfaces/user.ts | Removes IUserFormErrors (no longer used after migration). |
| frontend/hooks/useFormValidation.ts | Adds the new reusable form validation hook (dirty-gating, clear-on-focus, submit checkpoint, trimming, server error merge + toasts). |
| frontend/hooks/useFormValidation.tests.ts | Adds unit tests for hook behavior (dirty gating, submit checkpoint, trimming, server errors, in-flight guarding). |
| frontend/docs/patterns.md | Documents that useFormValidation implements the canonical validation rules and points to reference migrations. |
| frontend/components/buttons/Button/Button.tsx | Adds formId prop to support submitting a sibling <form> via the HTML form attribute. |
| .claude/rules/fleet-frontend.md | Updates validation guidance to prefer useFormValidation and references migrated forms. |
Files excluded by content exclusion policy (1)
- changes/use-form-validation-hook
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const commitFields = useCallback((changes: Partial<TFormData>) => { | ||
| const next = { ...formDataRef.current, ...changes }; | ||
| formDataRef.current = next; | ||
| setFormData(next); | ||
| Object.keys(changes).forEach((key) => dirtyFieldsRef.current.add(key)); | ||
| setIsDirty(true); | ||
|
|
||
| const currentErrors = validateRef.current(next); | ||
| setErrors((prev) => { | ||
| const kept: IFormErrors = {}; | ||
| let dropped = false; | ||
| Object.keys(prev).forEach((key) => { | ||
| if (currentErrors[key]) { | ||
| // Keep the message already on screen rather than the freshly computed | ||
| // one — a server error must not be overwritten by a client rule. | ||
| kept[key] = prev[key]; | ||
| } else { | ||
| dropped = true; | ||
| } | ||
| }); | ||
| return dropped ? kept : prev; | ||
| }); | ||
| }, []); |
…ApiUserForm - Return the request promise from onSubmit so the hook's double-submit guard actually engages; the reference migrations were swallowing it. - Extract getUserFieldErrors into userManagementHelpers so the same server reason reads identically from every screen that creates or edits a user. UsersPage still had pre-register copy, including a trailing period on a field error. - Add ApiUserForm tests: dirty gate, clear-on-focus, inline "at least one" errors, conditional clearing, trimming, free-tier rules, in-flight disabling. - Fix server errors being dropped by an unrelated commitFields, and the serverErrors effect looping on an inline object literal. - Keep every returned callback identity stable, enforced by a test.
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/use-form-validation-hook
Suppressed comments (1)
frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx:251
- For invite edits (
isInvitePending),IEditInviteFormDatarequirespassword: null, butbuildSubmitDatacurrently always setspasswordto the local state string even when the password field is hidden. This can sendpassword: ""to the invites API and break the request (and it’s inconsistent with the “hidden fields aren’t validated” rule above). Setpasswordtonullwhenever the password section isn’t shown.
email: data.email,
name: data.name,
newUserType: data.newUserType,
password: data.password,
sso_enabled: data.sso_enabled,
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/hooks/useFormValidation.ts (1)
242-264: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the server-error exemption for fields updated through
commitFields.
validateField(Line 295) andclearFieldError(Line 278) both remove a field fromserverErrorFieldsRefwhen the user interacts with it, so a stale server verdict does not outlive the field it was reported on.commitFieldsmarks the changed keys dirty (Line 246) but never removes them fromserverErrorFieldsRef.Text-input fields avoid the gap because their
onFocushandler callsclearFieldErrorbefore any edit happens. Committed-control fields (radios, dropdowns, multi-selects) have no equivalent step:commitFieldsis the only interaction point. If a server error is ever mapped onto a field driven bycommitFields, changing that field's value would not clear the exemption, so the stale server message would stay on screen until submit, reset, or an explicitclearFieldError/clearErrorscall.No current consumer (
UserForm,ApiUserForm) maps a server error onto acommitFields-driven field, so this has no active impact today. Since this hook is the shared source of truth for future form migrations, fix it now to prevent this from surfacing as a real bug later.🐛 Proposed fix
const commitFields = useCallback((changes: Partial<TFormData>) => { const next = { ...formDataRef.current, ...changes }; formDataRef.current = next; setFormData(next); - Object.keys(changes).forEach((key) => dirtyFieldsRef.current.add(key)); + Object.keys(changes).forEach((key) => { + dirtyFieldsRef.current.add(key); + serverErrorFieldsRef.current.delete(key); + }); setIsDirty(true);🤖 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 `@frontend/hooks/useFormValidation.ts` around lines 242 - 264, Update commitFields to remove every key in changes from serverErrorFieldsRef.current before recalculating errors, matching the cleanup behavior of validateField and clearFieldError while preserving the existing dirty-state and validation flow.frontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx (1)
66-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
addUserErrorswhen the Add User modal toggles.
toggleEditUserModalclearseditUserErrorson every toggle (Line 129), buttoggleAddUserModalandtoggleCreateUserModalnever clearaddUserErrors.AddUserModalfully unmounts whenshowCreateUserModalbecomes false (Line 438) and remounts fresh when it becomes true again, so its internaluseFormValidationstate resets to{}on each reopen.Because the server-error effect in
useFormValidationshows and toasts any field inserverErrorsthat differs from its freshly-reset internal state, a staleaddUserErrorsvalue from a previous failed submission (set at Line 222 or Line 250) reappears immediately on the next "Add user" open, before the user types anything. This produces a false inline error and a false toast on a blank form.Reset
addUserErrorsalongside the modal toggle, matching the pattern already used foreditUserErrors.🐛 Proposed fix
const toggleCreateUserModal = useCallback(() => { setShowCreateUserModal(!showCreateUserModal); setShowAddUserModal(false); + setAddUserErrors({}); }, [showCreateUserModal, setShowCreateUserModal, setShowAddUserModal]);Also applies to: 134-137
🤖 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 `@frontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx` around lines 66 - 75, Update the add-user modal toggle handlers, specifically toggleAddUserModal and toggleCreateUserModal, to clear addUserErrors whenever the modal is toggled. Match the existing reset behavior in toggleEditUserModal so reopening AddUserModal cannot reuse validation errors from a previous submission.
🤖 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.
Outside diff comments:
In `@frontend/hooks/useFormValidation.ts`:
- Around line 242-264: Update commitFields to remove every key in changes from
serverErrorFieldsRef.current before recalculating errors, matching the cleanup
behavior of validateField and clearFieldError while preserving the existing
dirty-state and validation flow.
In
`@frontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsx`:
- Around line 66-75: Update the add-user modal toggle handlers, specifically
toggleAddUserModal and toggleCreateUserModal, to clear addUserErrors whenever
the modal is toggled. Match the existing reset behavior in toggleEditUserModal
so reopening AddUserModal cannot reuse validation errors from a previous
submission.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 90edf46b-18b0-4e89-a3ff-ae6f16847da8
📒 Files selected for processing (11)
frontend/hooks/useFormValidation.tests.tsfrontend/hooks/useFormValidation.tsfrontend/pages/admin/ManageFleetsPage/TeamDetailsWrapper/UsersPage/UsersPage.tsxfrontend/pages/admin/ManageUsersPage/CreateApiUserPage/CreateApiUserPage.tsxfrontend/pages/admin/ManageUsersPage/CreateUserPage/CreateUserPage.tsxfrontend/pages/admin/ManageUsersPage/EditUserPage/EditUserPage.tsxfrontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tests.tsxfrontend/pages/admin/ManageUsersPage/components/ApiUserForm/ApiUserForm.tsxfrontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tests.tsxfrontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsxfrontend/pages/admin/ManageUsersPage/helpers/userManagementHelpers.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/pages/admin/ManageUsersPage/CreateUserPage/CreateUserPage.tsx
- frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx
- frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tests.tsx
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated no new comments.
Files excluded by content exclusion policy (1)
- changes/use-form-validation-hook
Suppressed comments (3)
frontend/pages/admin/ManageUsersPage/helpers/userManagementHelpers.ts:99
- Doc comment grammar: “a users/invites API failure” reads incorrectly. Consider rephrasing to “a users or invites API failure” (or similar) for clarity.
/**
* Maps a users/invites API failure to inline field errors, or null when the
* failure isn't field-specific and belongs in a toast instead.
*/
frontend/hooks/useFormValidation.ts:297
validateFieldvalidates against the raw (untrimmed) form data, buthandleSubmitvalidates against trimmed data. With validators likevalidEmail(no trimming), this can produce inconsistent UX (e.g., trailing spaces show an error on blur but submit succeeds). Consider validating blur against the same trimmed view (respectingskipTrim) to keep behavior consistent.
// Blur hands the field back to client validation, so a server verdict on it
// no longer applies.
serverErrorFieldsRef.current.delete(name);
const message = validateRef.current(formDataRef.current)[name];
setErrors((prev) => {
frontend/pages/admin/ManageUsersPage/components/UserForm/UserForm.tsx:545
- Password help text doesn’t mention the “at least 1 letter” requirement that the password validator enforces (and that the new invalid-format error copy mentions). This can confuse users who enter numbers+symbols only. Update the help text to match the actual rules.
type="password"
disabled={isSubmitting}
helpText="12-48 characters, with at least 1 number (e.g. 0 - 9) and 1 symbol (e.g. &*#)."
blockAutoComplete
Related issue: Resolves #48220, resolves #48269
Every Fleet form re-implements validation state by hand, so behavior drifts between them: errors appear on untouched fields, submit buttons disable while a field is invalid, and error copy is inconsistent. #49041 documented the target behavior but nothing implements it yet.
This adds a
useFormValidationhook as the single source of truth for those rules and migrates the new/edit user and API-only user forms onto it as the reference implementations. The remaining forms migrate in follow-ups.Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
For unreleased bug fixes in a release candidate, one of:
Database migrations
COLLATE utf8mb4_unicode_ci).New Fleet configuration settings
If you didn't check the box above, follow this checklist for GitOps-enabled settings:
fleetctl generate-gitopsfleetd/orbit/Fleet Desktop
runtime.GOOSis used as needed to isolate changesSummary by CodeRabbit
New Features
Bug Fixes