Skip to content

fix(webhooks): surface form validation errors and reset add-webhook drawer state#1736

Open
Shreyag02 wants to merge 1 commit into
mainfrom
fix/webhook-drawer-form-issues
Open

fix(webhooks): surface form validation errors and reset add-webhook drawer state#1736
Shreyag02 wants to merge 1 commit into
mainfrom
fix/webhook-drawer-form-issues

Conversation

@Shreyag02

Copy link
Copy Markdown
Contributor

Summary

Fixes several issues in the Add Webhook flow.
The originally reported bug — missing input field borders — was already fixed; while verifying, we found the "Add Webhook" form silently failed to submit, retained stale state on reopen, and could dead-end on certain responses.
This PR addresses those.

Changes

  • Render inline validation errors in CustomField so failed submissions show why (previously invisible).
  • Reset the create drawer form on open so previously entered values no longer persist.
  • Show an error toast when the create response contains no webhook, instead of silently doing nothing.
  • Set proper form defaultValues to avoid the controlled/uncontrolled input warning.
  • Fix the mismatched description length message ("Must be 10 or more characters" → 3, matching the min(3) rule) in both create and update.

Technical Details

  • The root cause of the "click does nothing / needs a second click" behavior was that CustomField only rendered Radix's native validity matchers (valueMissing, typeMismatch), which never fire for these constraint-less inputs. Validation is driven entirely by zod/react-hook-form, whose errors live in formState.errors and were never displayed — so an invalid URL or short description blocked handleSubmit with no feedback. Now uses useFormState({ control, name }) to render errors[name].message inline.
  • The create drawer stays permanently mounted (parent only toggles open), so the form was never reset. Added a reset on-open effect; the fix in CustomField also benefits the update drawer since it's shared.
  • Error styling uses a CSS module (custom-field.module.css) rather than inline styles, matching the existing PageHeader convention.

Test Plan

  • Manual testing completed
  • Build and type checking passes

SQL Safety (if your PR touches *_repository.go or goqu.*)

N/A

@Shreyag02 Shreyag02 requested a review from rohanchkrabrty July 3, 2026 11:30
@vercel

vercel Bot commented Jul 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
frontier Ready Ready Preview, Comment Jul 3, 2026 11:30am

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes
    • Added inline validation feedback for custom fields so form errors are shown more clearly.
    • Improved webhook creation by resetting the form each time the drawer opens, preventing stale values from reappearing.
    • Added a clearer error message when webhook creation fails.
    • Corrected validation text for webhook fields so messages match the actual requirements.

Walkthrough

Adds inline validation error rendering to the CustomFieldName component using react-hook-form's useFormState, with a new CSS error class. Updates webhook create form with explicit default values, a reset-on-open effect, corrected Zod messages, and an error toast for missing webhook responses; also fixes a Zod message in the update form.

Changes

Custom Field Error Display

Layer / File(s) Summary
Inline validation error rendering
web/sdk/admin/components/CustomField.tsx, web/sdk/admin/components/custom-field.module.css
Expands react-hook-form imports, derives an error message via useFormState, conditionally renders it as inline Text, and adds a .errorMessage CSS class using the danger color variable.

Webhook Form Fixes

Layer / File(s) Summary
Create webhook reset and error handling
web/sdk/admin/views/webhooks/webhooks/create/index.tsx
Adds useEffect import, sets explicit defaultValues, resets form fields when the drawer opens, corrects the description min-length validation message, and shows an error toast when the create response lacks a webhook object.
Update webhook validation message fix
web/sdk/admin/views/webhooks/webhooks/update/index.tsx
Corrects the url field's minimum-length error message text to match the enforced .min(3) constraint.

Estimated code review effort: 2 (Simple) | ~12 minutes

Suggested reviewers: rohanchkrabrty, rsbh

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
web/sdk/admin/views/webhooks/webhooks/create/index.tsx (1)

49-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Extract shared default values to avoid duplication.

The defaultValues object passed to useForm (Lines 51-56) and the object passed to methods.reset (Lines 63-68) are identical literals. Extracting a shared constant avoids drift if a field is added/renamed later.

♻️ Proposed refactor
+const DEFAULT_WEBHOOK_VALUES: NewWebhook = {
+  url: "",
+  description: "",
+  state: false,
+  subscribed_events: [],
+};
+
 export default function CreateWebhooks({ open = false, onClose: onCloseProp }: CreateWebhooksProps = {}) {
   ...
   const methods = useForm<NewWebhook>({
     resolver: zodResolver(NewWebookSchema),
-    defaultValues: {
-      url: "",
-      description: "",
-      state: false,
-      subscribed_events: [],
-    },
+    defaultValues: DEFAULT_WEBHOOK_VALUES,
   });

   useEffect(() => {
     if (open) {
-      methods.reset({
-        url: "",
-        description: "",
-        state: false,
-        subscribed_events: [],
-      });
+      methods.reset(DEFAULT_WEBHOOK_VALUES);
     }
   }, [open, methods.reset]);
web/sdk/admin/components/CustomField.tsx (1)

49-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Fragile indexing for nested field names.

errors?.[name] only works because current usages (url, description, subscribed_events, state) are flat field names. RHF's errors object mirrors nested/array field shapes, so a dotted name like user.address would resolve to undefined here even when an error exists (confirmed via RHF's own errors-object nesting behavior). Since CustomFieldNameProps.name is typed as a generic string for reuse, this is a latent trap for future nested usage.

getFieldState(name) is purpose-built for nested-safe field state retrieval and can be obtained from the same useFormState subscription.

♻️ Suggested fix
-  const { errors } = useFormState({ control, name });
-  const errorMessage = errors?.[name]?.message as string | undefined;
+  const { errors } = useFormState({ control, name });
+  const errorMessage = get(errors, name)?.message as string | undefined;

(or use useFormState({ control, name }).errors combined with RHF's get utility / getFieldState, which is explicitly documented for safely retrieving nested field state.)


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6684ed03-3065-4181-a91e-60030dcc2cc6

📥 Commits

Reviewing files that changed from the base of the PR and between 854c122 and 2c11b91.

📒 Files selected for processing (4)
  • web/sdk/admin/components/CustomField.tsx
  • web/sdk/admin/components/custom-field.module.css
  • web/sdk/admin/views/webhooks/webhooks/create/index.tsx
  • web/sdk/admin/views/webhooks/webhooks/update/index.tsx

@coveralls

Copy link
Copy Markdown

Coverage Report for CI Build 28657688531

Coverage remained the same at 44.865%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: No coverable lines changed in this PR.
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 37608
Covered Lines: 16873
Line Coverage: 44.87%
Coverage Strength: 12.49 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants