Added revalidation of inputs on change.#33
Conversation
Signed-off-by: Sajid Mannikeri <sajid.mannikeri@ad.infosys.com>
📝 WalkthroughWalkthroughAuthentication form components now expose ChangesPost-blur validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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
1 participant
Purpose
Approach
Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit