fix(form-core): don't auto-touch shifted siblings on array mutation#2225
Open
chatman-media wants to merge 1 commit into
Open
fix(form-core): don't auto-touch shifted siblings on array mutation#2225chatman-media wants to merge 1 commit into
chatman-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
Comment on lines
+1920
to
+1937
| 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.
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.
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