Frontend: document form validation rules and error copy register

[RachelElysia]

Issue

Docs-only. Kicks off the validation standardization workstream — target behavior for a forthcoming shared validation hook and per-form migrations.

Description

• Extends frontend/docs/patterns.md "Data validation" with 10 subsections: error timing, clearing, priority, submit button state, server-side errors, conditional validation, optional/disabled fields, input hygiene, in-flight lifecycle, visual affordances, and copy register.
• Adds rules under a new "Validation" section in .claude/rules/fleet-frontend.md.
• Describes target behavior; upcoming work will be done for a shared validation hook that applies these new patterns as well as per-form migrations

Screenrecording

Testing

• Reviewed markdown rendering on GitHub for both files
• QA'd all new/changed functionality manually

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.32%. Comparing base (703dcf0) to head (0ee1b9d).
⚠️ Report is 574 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #49041      +/-   ##
==========================================
+ Coverage   68.11%   68.32%   +0.21%     
==========================================
  Files        3731     3864     +133     
  Lines      235434   238315    +2881     
  Branches    12520    13285     +765     
==========================================
+ Hits       160371   162837    +2466     
- Misses      60670    61082     +412     
- Partials    14393    14396       +3     

Flag	Coverage Δ
frontend	61.29% <ø> (+1.99%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[RachelElysia]

Implementing this consistency in #49216

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates Fleet’s frontend documentation to standardize React form validation behavior and UX conventions (error timing/placement, submit behavior, copy).

Changes:

• Rewrites frontend/docs/patterns.md form validation guidance with detailed rules for error display/clearing, submit state, server errors, and lifecycle.
• Adds an authoritative pointer and summary checklist to .claude/rules/fleet-frontend.md to steer contributors toward the patterns doc.

Reviewed changes

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

File	Description
frontend/docs/patterns.md	Expands and clarifies form validation/UX rules (submit behavior, touched semantics, server errors, lifecycle, copy).
.claude/rules/fleet-frontend.md	Adds a “Validation” rules section that links to the authoritative patterns doc and summarizes key divergences.

---

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

[nulmete]

Agree, I think we should define dirty ("interacted-with" in L351 would fall into this category I think), touched, any other?

EDIT: we also mention isDirty below so I'd try to stay with just one term, either dirty or interacted-with to narrow down the cognitive load when reading this 😄

[RachelElysia]

Thanks for flagging!! I just addressed this by consolidating to a single dirty term with a definition line and called out the field-level dirty vs form-level isDirty distinction so they don't get conflated <3

[RachelElysia]

done

[RachelElysia]

done

[RachelElysia]

Updating so it shows on the confirmation.

[nulmete]

Agree w/ Copilot here. I think "Cancel" buttons in modals should cancel in-flight requests (I believe react-query does this behind the scenes IF we use useMutation for mutative operations... but we still have lots of occurrences where we use plain Promises, so worth double-checking).

[andymFleet]

Is cancelling requests possible for all actions? Is considering a confirmation dialog a good resolution to this problem for background instances, to ensure the user knows this will or will not cancel the request?

[RachelElysia]

Agree w/ Copilot here. I think "Cancel" buttons in modals should cancel in-flight requests (I believe react-query does this behind the scenes IF we use useMutation for mutative operations... but we still have lots of occurrences where we use plain Promises, so worth double-checking).

Just checked — we only use useMutation in 3 places (all in policies: resetPolicy, createPolicy, useUpdatePolicyAutomations). And FWIW useMutation doesn't actually auto-cancel in-flight mutations — react-query cancels queries on unmount, but mutations run to completion unless the mutationFn wires up an AbortController itself. So we'd be building this from scratch either way.

Agree it might be worth a future iteration. @mike-j-thomas has also floated dropping the Cancel button entirely (users find it confusing during submission), which might be the cleaner resolution than wiring abort everywhere. For now the doc calls out the resilience requirement so we don't ship the toast-after-unmount bug in the meantime.

Is cancelling requests possible for all actions? Is considering a confirmation dialog a good resolution to this problem for background instances, to ensure the user knows this will or will not cancel the request?

I added a line to address this:

+- The Cancel button remains enabled during submission and closes the modal immediately. It does not abort the in-flight reques
+t; the request completes in the background. We don't require abort because most call sites use plain Promises (not useMutatio +n), and we don't want a confirmation dialog on Cancel — it adds friction to the common case for a rare one.
411 +- Because Cancel doesn't abort, the submission must be resilient to the modal being closed before the request resolves. Guard
+post-success side effects (toast, navigation, cache invalidation) so they don't fire against an unmounted component or a scree
+n the user has already left. Failures on a closed modal are dropped silently — no toast, no re-open.

[nulmete]

Yeah, (auto-)canceling queries make sense, not mutations (I was wrong on the concept as well, since as soon as a mutative operation has begun, we'd also have to handle cancelation on the server which doesn't make much sense).

[RachelElysia]

removing thanks ai!

[RachelElysia]

@mike-j-thomas , @rachaelshaw , @marko-lisica please review at your earliest convenience slack thread

[marko-lisica]

@RachelElysia This is confusing. I understood this like we should show toast and then navigate, but I assume you're describing this:

- notify.success/notify.error defer the actual toast creation by one tick (setTimeout). So if a handler calls notify.success(...) and then router.push(...) synchronously, the dismiss-on-navigate listener fires first (clearing whatever was on the old page), and the new toast is created after — landing it on the destination page instead of getting wiped by its own navigation.
- Ordering matters and is load-bearing: call notify.success before router.push/router.replace, not after. Reversing the order can break the toast's auto-dismiss timer (see #48088).

The "Add software" flow (SoftwareCustomPackage.tsx:134-160) is the canonical example:
notify.success(<>Package successfully added.</>);   // called first
...
router.push(PATHS.SOFTWARE_TITLE_DETAILS(...));      // navigation after
So it uploads, calls notify.success immediately on API success, then pushes to the software title details page — the toast survives the route change and shows up there.

[RachelElysia]

Thanks for flagging! The "BEFORE" phrasing was ambiguous. I meant the code-call order, not the visual order.

For context, the notify.success defers the toast creation by a tick via setTimeout, so if you call it before router.push, the deferred toast survives the navigation and shows up on the destination page. If you call it after, the dismiss-on-navigate listener wipes it. SoftwareCustomPackage.tsx:134-160 is the canonical example.

Updating to clarify!

[marko-lisica]

@RachelElysia Is this supposed to display inline?

[RachelElysia]

@mike-j-thomas clarified all server side errors will appear in the toast message, but can/should appear inline as well if they are specific to a specific field

[nulmete]

Thanks for putting this up, I think it's going in a good direction 💯
LMK what you think about my comments below.

[nulmete]

Submit button stays enabled with invalid fields

We'll need to make sure we do not fire any API calls even though users can click it

[RachelElysia]

Updated to clarify the submit handler should not call the API if it finds clientside errors

[nulmete]

We should not revalidate on blur if the form is not dirty (i.e. we haven't typed anything yet), right? Otherwise we'll get all those red inputs when clicking in and out of an input field with nothing typed.

[RachelElysia]

Ok, this is what I modified to:

• Empty required field, user tabs through without typing → not dirty → no blur error, submit surfaces it ✓
• User types a, deletes back to empty, blurs → dirty (sticky) → presence error shows ✓
• Edit form pre-filled invalid, user tabs through without typing → not dirty → no error ✓
• Edit form user edits and reverts to initial → dirty (sticky) → validation still fires

[nulmete]

👍 I think this answers my comment above related to Re-validate on blur, never on keystroke.

[nulmete]

Agree, I think we should define dirty ("interacted-with" in L351 would fall into this category I think), touched, any other?

EDIT: we also mention isDirty below so I'd try to stay with just one term, either dirty or interacted-with to narrow down the cognitive load when reading this 😄

[nulmete]

Autofill would mean we interacted with the field(s). I think touched is implicitly included in interacted-with (as a user, whenever you want to interact with a field, i.e. type something, select a checkbox/radio, you have to touch it first)

Perhaps ^ is more of a "global" rule not specific to autofilling.

[RachelElysia]

Thank you! I consolidated the section around a single term. touched and "interacted-with" are both gone. Dirty is now the only field-level state name and it's defined as "once the user types into it or the browser autofills it" (sticky for the session). Autofill is folded into that definition rather than a separate bullet, so the redundancy you flagged goes away. Curious what you think of the updated version! Going to push shortly when I address everything

[nulmete]

When we define validation errors, you're saying that we should define something like a priority for each of them (if we have multiple)?

[RachelElysia]

Yeah, the only ones I have seen a second validation along with "is present", in which "is present" should always take priority

[nulmete]

This is already mentioned above in L354

[RachelElysia]

Good catch, dropping in the upcoming revision!

[nulmete]

Agree w/ Copilot here. I think "Cancel" buttons in modals should cancel in-flight requests (I believe react-query does this behind the scenes IF we use useMutation for mutative operations... but we still have lots of occurrences where we use plain Promises, so worth double-checking).

[Copilot]

🟡 Not ready to approve

The new “dirty” definition is internally inconsistent with the later rule that submit marks all fields dirty, which should be clarified to prevent divergent implementations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

frontend/docs/patterns.md:351

• The definition of field-level dirty says it only becomes dirty when the user types or the browser autofills, but a later bullet says submit "marks all fields dirty" (explicit exception). As written, that’s internally inconsistent and could lead to divergent implementations; update the dirty definition to include submit attempts (or rename the submit behavior).

A field is **dirty** once the user has typed into it or the browser has autofilled it. It stays dirty for the session, even if the value returns to its initial state. Field errors gate on `dirty`. Form-level `isDirty` ("form has changes") is a separate concept — see [Submit button state](#submit-button-state).

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

It introduces at least one non-clickable/incorrect doc reference link and a validation rule that is ambiguous enough to cause inconsistent implementations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (3)

.claude/rules/fleet-frontend.md:112

• The link to the validation patterns doc is likely broken in GitHub, because frontend/docs/patterns.md#data-validation is relative to .claude/rules/ and will resolve to .claude/rules/frontend/docs/patterns.md. Use a root-relative or correctly relative path so the rule can be followed easily.

**Read `frontend/docs/patterns.md#data-validation` before adding or editing form validation — that doc is authoritative.** Fleet diverges from what mainstream React libraries (Formik, react-hook-form, MUI, Ant Design) do by default on submit-button behavior, error timing, error position, and copy tone. Pattern-matching from another React app will land you in these specific mistakes:

frontend/docs/patterns.md:329

• This sentence reads like existing forms already follow the target behavior (“implement the rules directly”), but the next clause says they’re being migrated. Consider rewording to avoid implying that everything already matches the new rules.

The rules below describe the target behavior. Existing forms implement the rules directly and are migrated one at a time. New forms should follow these rules on day one.

frontend/docs/patterns.md:387

• This cross-field guidance is ambiguous about which blur triggers the mismatch check. If implemented on blur of either field, it would end up setting an error on a different field than the one being blurred (contradicting the earlier “blur validates that field only” rule). Clarify that mismatch validation runs on blur of the dependent/confirmation field.

- Cross-field checks (e.g. password + confirmation match) run on blur only when both fields are non-empty. If either is empty, skip the check — the empty field's own required-error covers it. On mismatch, attach the error to the dependent/confirmation field, not the source.

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Not ready to approve

The new validate example trims for presence but validates format against the untrimmed value, creating an inconsistency with the stated “trim before submit” hygiene rule.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Comments suppressed due to low confidence (1)

frontend/docs/patterns.md:342

• In the validate example, presence checking uses trim() but the format check uses the untrimmed value. This would treat trailing/leading whitespace as a format error even though the doc later recommends trimming before submit; using the trimmed value for both checks keeps the example consistent and avoids accidental whitespace validation bugs.

  if (!formData.email.trim()) {
    errors.email = "Enter your email";
  } else if (!isValidEmail(formData.email)) {
    errors.email = "Enter a valid email";
  }

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Low

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The changes are documentation-only, internally consistent, and remove a previously ambiguous reference without introducing broken links or contradictory guidance.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

Changes are documentation-only and the updated anchors/links and examples appear consistent and renderable based on the reviewed markdown context.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.