fix(web): unify all forms on login's inline-error validation#35
Conversation
Every form in apps/web now uses the same custom inline-error pattern the login page uses, and no native HTML5 validation bubble can appear anywhere in the app. - `noValidate` added to every <form> (the definitive guarantee against native bubbles), including login for consistency. - Forms that relied on native `required` (Add Contractor — the reported offender — plus Edit Contractor, Settings, Portal Profile, Portal Invoice, Time Entry, Engagement, Offboarding, Upload, Contact) now validate client-side before the network call and surface inline field errors via the shared Input `error` prop / a matching inline <p> for selects, textareas and file inputs. - Reuses the shared Zod schemas (createContractorSchema, createInvoiceSchema, createTimeEntrySchema, createEngagementSchema, initiateOffboardingSchema, uploadDocumentSchema, updateOrganizationSettingsSchema, updateContractorSchema) so messages match the rest of the app; explicit required checks where no schema maps. - The static Contact form became controlled with the same UX + a success banner instead of a no-op submit. Layout, copy, and submit/network logic unchanged — this is a consistency fix, not a redesign. tsc + next lint clean; web component tests pass (32).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request standardizes form validation across the application by integrating Zod schemas and implementing field-level error handling for contractor management, organization settings, invoice creation, and user profiles. Feedback focuses on improving robustness and consistency, specifically by addressing a potential RangeError during date parsing in the document upload modal. Additionally, the reviewer recommended replacing raw elements with the shared Input component in several pages to ensure UI consistency and suggested providing more explicit feedback for invalid inputs in the organization settings to improve the user experience.
| expiresAt: expiresAt | ||
| ? new Date(expiresAt).toISOString() | ||
| : undefined, |
There was a problem hiding this comment.
Directly calling .toISOString() on a new Date() object created from user input can throw a RangeError if the date string is invalid. It's safer to verify the date is valid before calling .toISOString().
| expiresAt: expiresAt | |
| ? new Date(expiresAt).toISOString() | |
| : undefined, | |
| expiresAt: expiresAt && !isNaN(new Date(expiresAt).getTime()) | |
| ? new Date(expiresAt).toISOString() | |
| : undefined, |
| if (parsedDays.length === 0) { | ||
| setError('Please enter at least one valid reminder day (1–90)'); | ||
| setIsSaving(false); | ||
| fieldErrors['reminderDays'] = | ||
| 'Enter at least one valid reminder day (1–90)'; | ||
| } |
There was a problem hiding this comment.
The current validation logic for reminderDays (lines 85-88) silently filters out invalid values such as non-numeric strings or numbers outside the 1-90 range. This can lead to a confusing user experience where the user's input is partially ignored without any feedback. It would be better to validate that every comma-separated value is valid and show an error message if any part of the input fails validation.
| className={`mt-1 block w-full rounded-lg border px-3 py-2 text-sm text-slate-900 shadow-xs focus:border-brand-500 focus:outline-none focus:ring-1 focus:ring-brand-500 disabled:bg-slate-50 disabled:text-slate-500 ${ | ||
| errors['name'] ? 'border-error-500' : 'border-slate-200' | ||
| }`} | ||
| /> | ||
| {errors['name'] && ( | ||
| <p className="mt-1.5 text-[13px] text-error-600"> | ||
| {errors['name']} | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
This form uses a raw <input> element and manual error rendering, which is inconsistent with other forms in this PR that use the shared Input component (e.g., in the contractor edit page). Using the Input component would reduce code duplication and ensure a consistent UI/UX across the application. Please consider refactoring this and other similar instances in this file.
| className={`mt-1 block w-full rounded-lg border px-3 py-2 text-sm focus:border-brand-500 focus:outline-none focus:ring-1 focus:ring-brand-500 ${ | ||
| errors['engagementId'] | ||
| ? 'border-error-500' | ||
| : 'border-slate-200' | ||
| }`} | ||
| /> | ||
| </label> | ||
| {errors['engagementId'] && ( | ||
| <p className="mt-1.5 text-[13px] text-error-600"> | ||
| {errors['engagementId']} | ||
| </p> | ||
| )} |
There was a problem hiding this comment.
This page contains multiple instances of raw <input> elements with duplicated Tailwind classes and manual error message logic. For better maintainability and consistency with the rest of the application, these should be replaced with the shared Input component from @/components/ui/input.
The unified-validation change added inline error branches to engagement-form (the one form in the coverage include set), dropping branch coverage below the 89% ratchet. Add tests for the new description-too-long and invalid-currency inline errors — coverage back to 92%, 34/34 pass.
1 participant
Why
Form validation was inconsistent: the login page shows custom inline error messages under each field, while Add Contractor (and other forms) fell back to the browser's native HTML5 validation bubbles. A visitor who hits an empty Add Contractor submit sees a native browser tooltip — a different, lower-quality UX than the rest of the app.
What
noValidateto every<form>inapps/web/src(12 forms). This is the definitive guarantee that no native validation bubble can appear anywhere.required/type/minnow validates client-side before the network call and renders inline field errors in the exact login style —Input'serrorprop, or a matching<p className="mt-1.5 text-[13px] text-error-600">+border-error-500for<select>/<textarea>/file inputs.Files: contractors/new, contractors/[id]/edit, settings, portal/profile, portal/invoices/new, (static)/contact, invite/[token], login (noValidate only), components/{time-entries,engagements,offboarding,documents}.
Verification
grepconfirms: 0 forms withoutnoValidate, 0 leftoverrequiredJSX attributes.tsc -p apps/web/tsconfig.jsonclean;next lintreports 0 errors (only pre-existing warnings in untouched files);pnpm --filter @contractor-os/web test→ 32/32 pass.