Fix input validation in React SDK.#22
Open
SajidMannikeri17 wants to merge 1 commit into
Open
Conversation
Signed-off-by: sajid.mannikeri <sajid.mannikeri@infosys.com>
📝 WalkthroughWalkthroughRecovery, SignIn, and SignUp base auth components now memoize their form field extraction via useMemo and rewrite server-side fieldErrors projection to build combined errors and touched maps, applying them in bulk via setTouchedFields and setFormErrors. SignIn additionally adds declarative rule-based field validation using buildValidatorFromRules. ChangesAuth form updates
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsx (1)
260-276: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared server-error projection helper
The server-error projection effect (clear → build errors/touched maps →
setTouchedFields→setFormErrors) is duplicated verbatim acrossBaseRecovery.tsx(lines 260-276),BaseSignIn.tsx(lines 377-392), andBaseSignUp.tsx(lines 489-505). Extracting a helper would prevent drift and simplify future maintenance.♻️ Proposed shared helper
+// packages/react/src/utils/projectServerFieldErrors.ts +import type {FieldError} from '`@thunderid/browser`'; + +interface FormErrorProjectionAPI { + clearErrors: () => void; + setTouchedFields: (touched: Record<string, boolean>) => void; + setErrors: (errors: Record<string, string>) => void; +} + +export function projectServerFieldErrors( + fieldErrors: FieldError[] | undefined, + api: FormErrorProjectionAPI, +): void { + api.clearErrors(); + if (!fieldErrors || fieldErrors.length === 0) { + return; + } + const errors: Record<string, string> = {}; + const touched: Record<string, boolean> = {}; + for (const fe of fieldErrors) { + if (!(fe.identifier in errors)) { + errors[fe.identifier] = fe.message; + touched[fe.identifier] = true; + } + } + api.setTouchedFields(touched); + api.setErrors(errors); +}Then each effect simplifies to:
useEffect(() => { - clearFormErrors(); - const responseFieldErrors: FieldError[] | undefined = (currentFlow?.data as any)?.fieldErrors; - if (!responseFieldErrors || responseFieldErrors.length === 0) { - return; - } - const errors: Record<string, string> = {}; - const touched: Record<string, boolean> = {}; - for (const fe of responseFieldErrors) { - if (!(fe.identifier in errors)) { - errors[fe.identifier] = fe.message; - touched[fe.identifier] = true; - } - } - setTouchedFields(touched); - setFormErrors(errors); + projectServerFieldErrors( + (currentFlow?.data as any)?.fieldErrors, + {clearErrors: clearFormErrors, setTouchedFields, setErrors: setFormErrors}, + ); }, [currentFlow, setFormErrors, setTouchedFields, clearFormErrors]);🤖 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/Recovery/BaseRecovery.tsx` around lines 260 - 276, The server-error projection logic is duplicated in BaseRecovery, BaseSignIn, and BaseSignUp, so extract the shared “clear → build errors/touched maps → setTouchedFields → setFormErrors” flow into a reusable helper. Create a helper that takes the current flow data and the form setters/clearer, then have the effects in BaseRecovery, BaseSignIn, and BaseSignUp call that helper instead of inlining the projection logic. Keep the behavior identical, including deduping by identifier and preserving the current effect dependencies.
🤖 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 `@packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsx`:
- Around line 260-276: The server-error projection logic is duplicated in
BaseRecovery, BaseSignIn, and BaseSignUp, so extract the shared “clear → build
errors/touched maps → setTouchedFields → setFormErrors” flow into a reusable
helper. Create a helper that takes the current flow data and the form
setters/clearer, then have the effects in BaseRecovery, BaseSignIn, and
BaseSignUp call that helper instead of inlining the projection logic. Keep the
behavior identical, including deduping by identifier and preserving the current
effect dependencies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 490ed76a-017a-418d-b755-e925987320f3
📒 Files selected for processing (3)
packages/react/src/components/presentation/auth/Recovery/BaseRecovery.tsxpackages/react/src/components/presentation/auth/SignIn/BaseSignIn.tsxpackages/react/src/components/presentation/auth/SignUp/BaseSignUp.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fix two defects in the input-validation feature introduced in #3301 that made client-side validation and server-side
fieldErrorsrendering unusable in real flows:BaseSignInandBaseRecoveryre-created theformFieldsarray on every render, which cascaded throughuseForm's memoized callbacks (getFieldConfig→validateField→setTouched) and re-fired the server-error projectionuseEffectevery render, triggeringMaximum update depth exceeded.BaseSignUpwas already patched withuseMemo; the other two were missed.fieldErrorssilently wiped from the UI —BaseSignIn,BaseSignUpandBaseRecoverymarked fields touched viauseForm'ssetTouched, which internally runsvalidateFieldand deletes the field's error from state when client-side rules pass. Whenever server-side rules were stricter than client-side rules (the common case), the server error was set and then immediately removed on the same render — the UI showed nothing.After this PR the SDK correctly renders both client-side validation errors on blur/submit and server-side
data.fieldErrorsreturned by the flow engine.Approach
1. Stabilise
formFieldsin everyuseForm-based Base componentuseForm'sgetFieldConfig,validateField, andsetTouchedall usefieldsin theiruseCallbackdependency arrays. Whenfieldsis a fresh array on every render, all three callbacks change reference on every render, which makes anyuseEffectdepending on them fire on every render. Wrapping the field extraction inuseMemo(keyed on[components, extractFormFields]) freezes the reference until the flow's components actually change, breaking the loop.Applied to
BaseSignIn.tsxandBaseRecovery.tsx.BaseSignUp.tsxalready had this fix.2. Use
setTouchedFieldsinstead ofsetTouchedwhen projecting server errorsuseFormexposes two ways to mark fields as touched:setTouched(name, isTouched)— sets touched and re-runsvalidateField(name), mutatingerrorsbased on client-side rules. Intended for blur/change handlers where re-validation is desired.setTouchedFields(fieldsMap)— bulk state setter that only updates touched, no validation triggered.The server-error projection
useEffectwas callingsetTouched, which caused the client-side revalidation to overwrite the server error. Switched tosetTouchedFieldsin the threeuseForm-based Base components.setTouchedis still used inline inhandleInputBlurwhere re-validation is the intended behaviour.3. Scope
BaseAcceptInviteandBaseInviteUsermanageformErrorsandtouchedFieldsvia plainuseState— notuseForm— so neither defect applies to them and they are unchanged.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit