Add onURLBlur handler for InputField - #48854
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48854 +/- ##
==========================================
+ Coverage 68.03% 68.07% +0.04%
==========================================
Files 3689 3689
Lines 234205 234210 +5
Branches 12489 12341 -148
==========================================
+ Hits 159347 159446 +99
+ Misses 60543 60449 -94
Partials 14315 14315
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:
|
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 updates the software automations “Manage automations” modal so the Webhook “Destination URL” field validates when the user leaves the field (on blur), matching the behavior of other URL inputs across the Fleet UI.
Changes:
- Add an
onURLBlurhandler that runsvalidateWebhookURL(destinationUrl)and updateserrors.url. - Wire the new handler into the Webhook “Destination URL”
<InputField onBlur={...} />.
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tsx | Adds on-blur URL validation for the webhook destination URL input. |
| changes/40410-webhook-url-validation-onblur.md | User-visible change note (excluded from diff by policy; not reviewed). |
Files excluded by content exclusion policy (1)
- changes/40410-webhook-url-validation-onblur.md
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis change adds blur-based validation for the Destination URL field in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DestinationURLField
participant onURLBlur
participant errorsState
User->>DestinationURLField: blur event
DestinationURLField->>onURLBlur: onBlur
onURLBlur->>onURLBlur: check softwareAutomationsEnabled
onURLBlur->>onURLBlur: validateWebhookURL(destinationUrl)
onURLBlur->>errorsState: replace url error
errorsState-->>DestinationURLField: render validation state
Estimated code review effort: Medium Related issues: Suggested labels: frontend, bug Suggested reviewers: none identified Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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.
🧹 Nitpick comments (1)
frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tsx (1)
230-238: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider consolidating duplicated validation/merge logic.
onURLBlurandhandleSaveAutomationboth callvalidateWebhookURL(destinationUrl)and merge the result intoerrorswith slightly different merge patterns (omit+ spread vs. conditionalomit). Extracting a sharedapplyWebhookUrlErrorshelper would reduce duplication and keep the two call sites in sync if validation logic changes.♻️ Example consolidation
+ const applyWebhookUrlErrors = (webhookErrors: { [key: string]: string }) => { + setErrors((prevErrs) => ({ ...omit(prevErrs, "url"), ...webhookErrors })); + }; + const onURLBlur = () => { if (!softwareAutomationsEnabled) { return; } const { errors: webhookErrors } = validateWebhookURL(destinationUrl); - setErrors((prevErrs) => ({ ...omit(prevErrs, "url"), ...webhookErrors })); + applyWebhookUrlErrors(webhookErrors); };Also applies to: 240-251
🤖 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/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tsx` around lines 230 - 238, The webhook URL validation and error-merging logic is duplicated between onURLBlur and handleSaveAutomation in ManageSoftwareAutomationsModal, so extract a shared helper such as applyWebhookUrlErrors around validateWebhookURL(destinationUrl). Update both call sites to use the helper consistently instead of each doing its own omit/spread merge, keeping the error shape and behavior in sync if validation changes.
🤖 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.
Nitpick comments:
In
`@frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tsx`:
- Around line 230-238: The webhook URL validation and error-merging logic is
duplicated between onURLBlur and handleSaveAutomation in
ManageSoftwareAutomationsModal, so extract a shared helper such as
applyWebhookUrlErrors around validateWebhookURL(destinationUrl). Update both
call sites to use the helper consistently instead of each doing its own
omit/spread merge, keeping the error shape and behavior in sync if validation
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0c99fed-0159-4204-bcdc-2583a24b99fe
⛔ Files ignored due to path filters (1)
changes/40410-webhook-url-validation-onblur.mdis excluded by!**/*.md
📒 Files selected for processing (1)
frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tests.tsx (2)
47-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo coverage for the disabled-field early return in
onURLBlur.Per the upstream handler (
ManageSoftwareAutomationsModal.tsx:230-238), blur validation is skipped whensoftwareAutomationsEnabledis false. Consider adding a test with the webhook disabled to lock in that no error surfaces on blur in that state.🤖 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/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tests.tsx` around lines 47 - 106, Add test coverage for the disabled-state early return in onURLBlur: when softwareAutomationsEnabled is false, blurring the destination URL field should skip validation and not surface either INVALID_URL_ERROR or REQUIRED_URL_ERROR. Extend the ManageSoftwareAutomationsModal tests by rendering the modal in the disabled webhook state and asserting no error appears after blur, using the existing renderModal helper and URL placeholder assertions for location.
55-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer accessible queries over
getByPlaceholderText.Placeholder text is presentational and can change independently of the field's semantics; querying by label/role (e.g.
getByLabelText(/destination url/i)) is more resilient and better reflects how users/assistive tech identify the field.🤖 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/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tests.tsx` around lines 55 - 104, Update the ManageSoftwareAutomationsModal tests to query the URL field via an accessible selector instead of getByPlaceholderText; replace uses of URL_PLACEHOLDER in these cases with a label- or role-based query that matches the field’s accessible name (for example, the destination URL label). Keep the existing validation assertions unchanged and update any related test helpers in ManageSoftwareAutomationsModal.tests.tsx to use the same accessible lookup consistently.
🤖 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.
Nitpick comments:
In
`@frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tests.tsx`:
- Around line 47-106: Add test coverage for the disabled-state early return in
onURLBlur: when softwareAutomationsEnabled is false, blurring the destination
URL field should skip validation and not surface either INVALID_URL_ERROR or
REQUIRED_URL_ERROR. Extend the ManageSoftwareAutomationsModal tests by rendering
the modal in the disabled webhook state and asserting no error appears after
blur, using the existing renderModal helper and URL placeholder assertions for
location.
- Around line 55-104: Update the ManageSoftwareAutomationsModal tests to query
the URL field via an accessible selector instead of getByPlaceholderText;
replace uses of URL_PLACEHOLDER in these cases with a label- or role-based query
that matches the field’s accessible name (for example, the destination URL
label). Keep the existing validation assertions unchanged and update any related
test helpers in ManageSoftwareAutomationsModal.tests.tsx to use the same
accessible lookup consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ae4f086-aefd-412c-8938-6c5888c66c1f
📒 Files selected for processing (1)
frontend/pages/SoftwarePage/components/modals/ManageSoftwareAutomationsModal/ManageSoftwareAutomationsModal.tests.tsx
| }; | ||
|
|
||
| const onURLBlur = () => { | ||
| // Skip validation whenever the field is disabled (automations off or GitOps |
There was a problem hiding this comment.
Going to note that this should be a documented pattern
| expect(urlInput).toBeDisabled(); | ||
|
|
||
| // The field is read-only in GitOps mode, so a blur must not surface an error. | ||
| fireEvent.blur(urlInput); |
There was a problem hiding this comment.
TODO @RachelElysia: update docs/Contributing/guides/ui/fleet-ui-testing.md to explicitly state the userEvent-over-fireEvent convention examples already demonstrate const { user } = renderWithSetup(...), but there's no prose rule. Add a short note covering when fireEvent is still warranted (events with no user-gesture equivalent like scroll/resize/visibilitychange, and defense-in-depth guard tests like blurring a disabled input).
RachelElysia
left a comment
RachelElysia
left a comment
There was a problem hiding this comment.
Looks like it's not building because of test type errors
…p) (#49932) <!-- Add the related story/sub-task/bug number, like Resolves #123, or remove if NA --> **Related issue:** Resolves #40410 Follow-up to #48854, which added on-blur validation to the software vulnerability automations webhook. QA found three more forms with inconsistent validation, in two flavors: - **Validated only on save** (no on-blur feedback): Policies > Manage automations > Other workflows > Destination URL. - **Errored before any input** (validation fired on mount/enable): Settings > Users > Add/edit user > Email, and the host status webhook Destination URL (both global and fleet settings). This makes them consistent with the rest of the app: no error on open/enable → validate on blur → clear the field's error as the user edits → validate on submit. - `OtherWorkflowsModal` — added an on-blur handler for the Destination URL (guarded by the field's disabled condition). - `UserForm` — on-blur now validates only the blurred field (so blurring the autofocused Name no longer flags the empty Email/Password); submit validates all fields. - `GlobalHostStatusWebhook` — removed the `useEffect` that validated the moment the webhook was enabled; validation now runs on blur and submit. - `TeamSettings` (fleet host status webhook) — the Destination URL error is no longer surfaced on change/enable; it validates on blur and submit. # Checklist for submitter - [x] Changes file added for user-visible changes in `changes/`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated (URL and email fields validate on blur and on submit; no errors are shown before the user interacts). ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved webhook destination URL validation across integrations, team settings, and workflow automation forms. * Validation messages now appear after leaving the URL field, rather than prematurely while enabling or editing. * Prevented saving or submitting forms with missing or invalid destination URLs. * Correctly clears validation errors once a valid URL is entered. * Improved user form validation so field-specific errors appear only for the field being reviewed, while submit continues to validate the full form. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…rms (#40410 follow-up) (#50129) Cherry-pick of #49932 into the 4.90 RC branch. Second of two PRs for #40410. The first (#48854) is already present in `rc-minor-fleet-v4.90.0` (merged before the branch was cut); this brings in the follow-up so the fix ships complete in 4.90. Applied cleanly with no conflicts.
Related issue: Resolves #40410
Checklist for submitter
changes/. See Changes files for more information.Testing
qa-40410.webm
Summary by CodeRabbit