fix(form-core): don't auto-touch shifted siblings on array mutation - #2225
fix(form-core): don't auto-touch shifted siblings on array mutation#2225chatman-media wants to merge 1 commit into
Conversation
validateArrayFieldsStartingFrom re-validates the array items shifted by an insert/remove/replace. It went through validateField, which touches each field as a side-effect, so siblings the user never interacted with were flipped to isTouched: true. The shifted fields still need validating (their value changed), so keep calling validateField, but restore the prior touched state for fields that weren't touched before the call. Closes TanStack#2131
📝 WalkthroughWalkthroughArray field validation now preserves touched state for shifted siblings while still returning validation errors. Regression tests cover forwarded array validation and ChangesArray shift touched-state fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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.
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/form-core/src/FormApi.ts`:
- Around line 1920-1937: The shifted-field validation in FormApi still goes
through validateField, which auto-touches mounted fields and can trigger
touch-gated side effects before the flag is restored. Update the validation flow
used for array-shifted fields in FormApi so it bypasses the isTouched mutation
entirely for fields that were not previously touched, while still running
validation and preserving existing mounted-field error behavior. Use the
fieldInstance / wasTouched logic around validateField as the entry point, and
add a regression test covering remove/insert/replace cases to ensure on-mount
errors remain intact without spuriously touching siblings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6942c34d-2adf-42d1-99e9-705feb7ee690
📒 Files selected for processing (3)
packages/form-core/src/FormApi.tspackages/form-core/tests/FieldGroupApi.spec.tspackages/form-core/tests/FormApi.spec.ts
| Promise.resolve().then(() => { | ||
| // These fields were merely shifted by an array mutation, the user | ||
| // never interacted with them. Their value did change, so they | ||
| // still need to be validated, but `validateField` auto-touches as | ||
| // a side-effect — which would spuriously mark untouched siblings | ||
| // as touched. Remember the prior touched state and restore it for | ||
| // fields that weren't touched before. | ||
| const fieldInstance = this.fieldInfo[nestedField]?.instance | ||
| const wasTouched = fieldInstance?.store.state.meta.isTouched ?? true | ||
|
|
||
| const result = this.validateField(nestedField, cause) | ||
|
|
||
| if (fieldInstance && !wasTouched) { | ||
| fieldInstance.setMeta((prev) => ({ ...prev, isTouched: false })) | ||
| } | ||
|
|
||
| return result | ||
| }), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This still runs the auto-touch side effect through validateField.
Line 1930 still calls validateField, and that method sets isTouched = true for mounted fields before validating on Lines 1967-1971. Restoring false afterward only fixes the final flag; it does not prevent touch-gated side effects that can fire during that window (for example the shouldInvalidateOnMount path later in this file). To fully match the PR objective, shifted mounted fields need a validation path that bypasses the touch mutation entirely, plus a regression test that preserves existing on-mount errors after remove/insert/replace.
🤖 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/form-core/src/FormApi.ts` around lines 1920 - 1937, The
shifted-field validation in FormApi still goes through validateField, which
auto-touches mounted fields and can trigger touch-gated side effects before the
flag is restored. Update the validation flow used for array-shifted fields in
FormApi so it bypasses the isTouched mutation entirely for fields that were not
previously touched, while still running validation and preserving existing
mounted-field error behavior. Use the fieldInstance / wasTouched logic around
validateField as the entry point, and add a regression test covering
remove/insert/replace cases to ensure on-mount errors remain intact without
spuriously touching siblings.
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
Checked validateField to make sure the touch/restore sequence here can't race against it. It marks the field touched synchronously, before returning, if (!fieldInstance.store.state.meta.isTouched) { fieldInstance.setMeta(...isTouched: true) } runs and completes before fieldInstance.validate(cause) is even called. So by the time this wrapper's this.validateField(nestedField, cause) call returns control, the touch has already happened synchronously, and restoring isTouched: false right after is deterministic, it isn't racing against anything async, since the actual async validation work (if any) happens later off the returned promise and doesn't touch isTouched again itself.
That synchronous force-touch inside validateField is also why this couldn't just skip touching in the first place, FieldApi.validate() bails out early with if (!this.state.meta.isTouched) return [], so a shifted field genuinely needs to be touched for its validators to run at all when its value changed out from under it. Temporarily touching it to get validation to execute, then restoring the field to untouched if it wasn't touched going in, is the right way to reconcile "this field's value legitimately changed and needs revalidating" with "the user never interacted with it."
Good that the updated FieldGroupApi.spec.ts test adds a real onChange validator to field1 and asserts errors came back ['validated'] in addition to checking isTouched stayed false, that's what actually proves validation still ran on the shifted field rather than the fix silently skipping it to avoid the touch side effect.
2 participants
Closes #2131
Problem
form.removeValue(index)(andinsertValue/replaceValue) flipsisTouchedtotrueon every sibling that shifts, even when the user never interacted with them. Because the bug is inform-core, every adapter is affected. The maintainer confirmed the touch "makes little sense" in #2131.Cause
After an array mutation,
validateArrayFieldsStartingFromre-validates the shifted items. It routes each one throughvalidateField, which auto-touches the field as a side-effect (so thatFieldApi.validate's pristine guard doesn't short-circuit). The shifted siblings genuinely need re-validating — their value changed — but they should not be marked touched.Fix
A prior attempt (#2133) was closed as overcomplicated; the maintainer hinted the fix should target a different function. This keeps it entirely inside
validateArrayFieldsStartingFrom(the function that owns the shift-revalidation), with no new option threading: remember each mounted field's priorisTouched, still callvalidateFieldso validation runs, then restoreisTouched: falsefor the fields that weren't touched before. Fields that were touched, and form-level validation for unmounted fields, are unchanged.Tests
form-coreregression test: removing an item from a 5-element array leaves all remaining siblingsisTouched: false. Fails without the fix ([false, false, true, true]).FieldGroupApiforwarding tests that previously asserted the buggy auto-touch. The dedicated one now also asserts the shifted field's validator actually ran (errors === ['validated']), so it still proves forwarding while verifying no spurious touch.form-core(499) andreact-form(123) suites pass; eslint, prettier and type checks (TS 5.4–5.9) clean.Summary by CodeRabbit