Added revalidation of inputs on change.#33
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAuthentication form components now expose ChangesAuthentication form behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AuthForm
participant useForm
User->>AuthForm: type after blur
AuthForm->>useForm: setValue(nextValue)
useForm->>useForm: validate touched field
useForm-->>AuthForm: update or clear error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx (1)
591-595: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove hardcoded
thunderidliteral fromdata-testidattributes.As per path instructions, avoid hardcoding the vendor name
thunderidin DOMdata-*attributes. Since this is an internal test identifier and the brand prefix is likely not load-bearing, the best fix is to avoid the vendor prefix entirely and usedata-testid="signin". If a brand-scoped namespace is genuinely required by external testing frameworks, use${getVendorPrefix(vendor)}-signininstead.
packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx#L591-L595: replacedata-testid="thunderid-signin"withdata-testid="signin".packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx#L601-L606: replacedata-testid="thunderid-signin"withdata-testid="signin".packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx#L618-L623: replacedata-testid="thunderid-signin"withdata-testid="signin".packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx#L634-L639: replacedata-testid="thunderid-signin"withdata-testid="signin".🤖 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 `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx` around lines 591 - 595, Remove the hardcoded vendor prefix from all four data-testid attributes in BaseSignIn: packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx at lines 591-595, 601-606, 618-623, and 634-639. Set each identifier to the unprefixed signin value; no other rendering behavior needs to change.Source: Path instructions
🧹 Nitpick comments (1)
packages/react/src/hooks/useForm.ts (1)
320-329: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrevent unnecessary state updates when the field error remains unchanged.
When
shouldValidateis true,setFormErrorscurrently creates and returns a new object on every keystroke, even if the error for the field hasn't changed. Returning the existing state object when there's no change allows React to bail out of unnecessary re-renders.♻️ Proposed refactor to bail out of state updates
const error: string | null = computeFieldError(value, getFieldConfig(name), requiredMessage); setFormErrors((prev: Record<keyof T, string>) => { + if (prev[name] === error || (!error && !(name in prev))) { + return prev; + } const newErrors: Record<keyof T, string> = {...prev}; if (error) { newErrors[name] = error;🤖 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 `@packages/react/src/hooks/useForm.ts` around lines 320 - 329, Update the setFormErrors callback in the shouldValidate path to compare the current error for name with the newly computed error before cloning state. Return the existing prev object when the field’s error is unchanged; otherwise preserve the current add/remove behavior using a copied error map.
🤖 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 `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx`:
- Around line 591-595: Remove the hardcoded vendor prefix from all four
data-testid attributes in BaseSignIn:
packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx at lines
591-595, 601-606, 618-623, and 634-639. Set each identifier to the unprefixed
signin value; no other rendering behavior needs to change.
---
Nitpick comments:
In `@packages/react/src/hooks/useForm.ts`:
- Around line 320-329: Update the setFormErrors callback in the shouldValidate
path to compare the current error for name with the newly computed error before
cloning state. Return the existing prev object when the field’s error is
unchanged; otherwise preserve the current add/remove behavior using a copied
error map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 50a0fcd0-cae2-4355-8cd9-b32884e12d2a
📒 Files selected for processing (5)
packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsxpackages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsxpackages/react/src/components/presentation/auth/SignIn/SignIn.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpackages/react/src/hooks/useForm.ts
3617dcf to
50b522c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx (1)
282-294: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
acceptableTypeoutside the component.Recreating this array on every render violates React dependency rules because it is referenced inside the
extractFormFieldsuseCallbackbut omitted from its dependency array. Moving the constant outside the component resolves the dependency issue and avoids unnecessary allocations.Define the array outside the component:
const ACCEPTABLE_TYPES = [ 'TEXT_INPUT', 'PASSWORD_INPUT', 'EMAIL_INPUT', 'PHONE_INPUT', 'OTP_INPUT', 'SELECT', 'DATE_INPUT', 'CUSTOM', ];♻️ Proposed refactor
- /** - * Component type for which forms validation will be applicable - */ - const acceptableType = [ - 'TEXT_INPUT', - 'PASSWORD_INPUT', - 'EMAIL_INPUT', - 'PHONE_INPUT', - 'OTP_INPUT', - 'SELECT', - 'DATE_INPUT', - 'CUSTOM', - ];And update its usage below:
- if (acceptableType.includes(component.type)) { + if (ACCEPTABLE_TYPES.includes(component.type)) {🤖 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 `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx` around lines 282 - 294, Move the acceptableType array out of the component scope, rename it to ACCEPTABLE_TYPES, and update extractFormFields and any other references to use the module-level constant so useCallback dependencies remain stable.
🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx`:
- Around line 447-466: Update handleTerminalResponse so terminal error responses
clear state, set the error, and clean up URL parameters, then return true
instead of throwing. Preserve the existing recoverable-error behavior and ensure
initializeFlow and handleSubmit do not enter their catch paths for this handled
terminal response.
---
Nitpick comments:
In `@packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx`:
- Around line 282-294: Move the acceptableType array out of the component scope,
rename it to ACCEPTABLE_TYPES, and update extractFormFields and any other
references to use the module-level constant so useCallback dependencies remain
stable.
🪄 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: caba69a6-d5ce-4c88-8470-bf439859d14b
📒 Files selected for processing (5)
packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsxpackages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsxpackages/react/src/components/presentation/auth/SignIn/SignIn.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpackages/react/src/hooks/useForm.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx
- packages/react/src/hooks/useForm.ts
1d96bcf to
b0ce359
Compare
|
@SajidMannikeri17 did we test that this fix works and won't cause other issues like call depth exceeding? |
|
Yes @ThaminduDilshan. We have tested these changes locally via symlink. There are no issues. |
|
Hey @SajidMannikeri17, Seems like the formatting linter is failing. |
b0ce359 to
e1d4487
Compare
|
Hi @brionmario. Fixed the linter. Please check. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/react/src/components/presentation/auth/SignIn/SignIn.tsx`:
- Around line 606-607: Restrict the initializationAttemptedRef reset in
handleTerminalResponse to unrecoverable Error responses only. Do not reset the
gate for Complete responses, including successful completion without a redirect
URL, so clearing flow state cannot trigger a new sign-in flow.
🪄 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: 3cf33985-1f7b-4ee7-9ecc-b3e252d9ba52
📒 Files selected for processing (5)
packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsxpackages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsxpackages/react/src/components/presentation/auth/SignIn/SignIn.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsxpackages/react/src/hooks/useForm.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsx
- packages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx
- packages/react/src/hooks/useForm.ts
- packages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsx
Signed-off-by: Sajid Mannikeri <[email protected]>
e1d4487 to
b6956ae
Compare
3 participants
Purpose
Fixes thunder-id/thunderid#3145. Inline validation errors in
SignIn/SignUp/Recoverycurrently persist while the user corrects their input and only clear on blur. Adds an opt-in proprevalidateOnChangeAfterBluron those components so a rendered error clears the moment the value becomes valid. Also fixes a stale-valuesclosure bug insideuseFormthat would otherwise cause a one-keystroke lag when on-change validation runs.Approach
useFormrefactorcomputeFieldError(value, fieldConfig, requiredMessage)as a pure helper — takes the value as an argument instead of reading a closed-overvaluesstate.setValuenow validates against the next value viacomputeFieldError, fixing the one-keystroke lag caused byvalidateFieldreading pre-update state aftersetFormValues(prev => …)queued its update.revalidateOnChangeAfterBlur?: boolean(defaultfalse). When enabled,setValuere-runs validation only iftouched[name] === true, so errors don't appear while initially typing — only while correcting after the first blur.validateField(blur path) unchanged in behavior; delegates tocomputeFieldErrorinternally.Prop plumbing
revalidateOnChangeAfterBlur?: boolean(defaultfalse) toBaseSignInProps,BaseSignUpProps,BaseRecoveryProps; passed to their internaluseFormcalls.SignUp/Recoveryinherit the prop via existingBaseXProps &intersections +{...rest}forwarding.SignInuses an explicitSignInProps, so the prop was added and forwarded to<BaseSignIn>explicitly.Backward compatibility
falseat every layer — no consumer sees a change unless they opt in.validateOnChangeorrevalidateOnChangeAfterBlur.validateOnChangeandrevalidateOnChangeAfterBlurare independent.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary by CodeRabbit
New Features
revalidateOnChangeAfterBlurflag for sign-in, sign-up, and account recovery forms to re-run validation on subsequent typing after a blur event (defaults to preserving existing behavior).Bug Fixes
Behavior Changes