fix(form-core): prevent removeValue from auto-touching shifted siblings#2133
Open
mixelburg wants to merge 2 commits intoTanStack:mainfrom
Open
fix(form-core): prevent removeValue from auto-touching shifted siblings#2133mixelburg wants to merge 2 commits intoTanStack:mainfrom
mixelburg wants to merge 2 commits intoTanStack:mainfrom
Conversation
When calling form.removeValue(index) on an array field, isTouched was incorrectly set to true on every sibling at index >= removed index, even when the user never interacted with those fields. The fix bypasses validateField for fields with existing instances and calls fieldInstance.validate() directly, which preserves validation without the auto-touch side-effect. Fixes TanStack#2131
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FormApi.ts`:
- Around line 1608-1616: The current code calls fieldInstance.validate(cause)
which avoids auto-touching but relies on FieldApi.validate returning [] when
meta.isTouched is false, leaving shifted untouched siblings unvalidated; change
this by adding an internal "validate without touching" path: extend
validateField to accept an options flag (e.g., { touch?: boolean }) or add a new
internal method (e.g., validateFieldInternal/FieldApi.validateWithoutTouch) that
runs the same validation logic regardless of meta.isTouched but does not mutate
touch state, then replace the direct call to fieldInstance.validate(cause) with
a call to validateField(nestedField, cause, { touch: false }) or the new
internal method so shifted siblings revalidate without auto-touch side effects;
update FieldApi.validate to delegate to the shared internal validator to avoid
duplication.
🪄 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: d506b7d4-0546-43d1-a8b3-a5847867e52b
📒 Files selected for processing (1)
packages/form-core/src/FormApi.ts
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 1m 26s | View ↗ |
nx run-many --target=build --exclude=examples/** |
✅ Succeeded | 34s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-04-20 05:38:36 UTC
The previous fix bypassed validateField for all calls to validateArrayFieldsStartingFrom, which broke: - validateArrayFieldsStartingFrom when called directly (expected isTouched) - replaceFieldValue (expected errors to appear on validation) - removeFieldValue (expected errors to appear on validation) The fix now uses an avoidTouch option that: 1. Only prevents permanently marking fields as touched during removeFieldValue 2. Temporarily sets isTouched=true to allow validation to run (so errors are still collected), then restores the original isTouched state 3. All other callers use the default behavior (validateField with touch)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/form-core/src/FormApi.ts (1)
1579-1586:FieldGroupApi.validateArrayFieldsStartingFromdoes not forwardoptions.Per
packages/form-core/src/FieldGroupApi.ts:310-324, theFieldGroupApiwrapper delegates tothis.form.validateArrayFieldsStartingFrom(name, index, cause)without a 4th argument. Any caller (internal or external) going through a FieldGroup will silently loseavoidTouch, so if this helper is ever invoked from a FieldGroup-scoped removal path it will regress to the old auto-touch behavior. Update FieldGroupApi's signature and forwardoptionsfor parity.validateArrayFieldsStartingFrom = async < TField extends DeepKeysOfType<TFieldGroupData, any[]>, >( field: TField, index: number, cause: ValidationCause, + options?: { avoidTouch?: boolean }, ) => { return this.form.validateArrayFieldsStartingFrom( this.getFormFieldName(field), index, cause, + options, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/src/FormApi.ts` around lines 1579 - 1586, The FieldGroupApi.validateArrayFieldsStartingFrom wrapper currently omits the options parameter so avoidTouch is lost; update FieldGroupApi's method signature to accept the fourth parameter (options?: { avoidTouch?: boolean }) and forward it when calling this.form.validateArrayFieldsStartingFrom(name, index, cause, options) so callers via FieldGroup retain avoidTouch behavior—ensure the method declaration and the delegated call both include the options parameter and types match the FormApi.validateArrayFieldsStartingFrom signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FormApi.ts`:
- Around line 1609-1634: The temporary-touch logic inside the Promise.resolve
block when options?.avoidTouch is true should ensure the original isTouched is
always restored even if fieldInstance.validate(cause) throws or rejects: wrap
the validate call and the second setMeta restoration in a try/finally so that
after temporarily setting fieldInstance.setMeta(... isTouched: true) you always
run the restore setMeta(...) in the finally block; keep the early-return of the
validate result, and otherwise fall back to this.validateField(nestedField,
cause). Reference the Promise.resolve(...) block, options?.avoidTouch,
fieldInstance, fieldInstance.validate(cause), and fieldInstance.setMeta(...)
when making the change.
---
Nitpick comments:
In `@packages/form-core/src/FormApi.ts`:
- Around line 1579-1586: The FieldGroupApi.validateArrayFieldsStartingFrom
wrapper currently omits the options parameter so avoidTouch is lost; update
FieldGroupApi's method signature to accept the fourth parameter (options?: {
avoidTouch?: boolean }) and forward it when calling
this.form.validateArrayFieldsStartingFrom(name, index, cause, options) so
callers via FieldGroup retain avoidTouch behavior—ensure the method declaration
and the delegated call both include the options parameter and types match the
FormApi.validateArrayFieldsStartingFrom signature.
🪄 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: e842bc31-0829-4659-8dff-b4c5f0928b2e
📒 Files selected for processing (1)
packages/form-core/src/FormApi.ts
Comment on lines
+1609
to
+1634
| Promise.resolve().then(async () => { | ||
| // If avoidTouch is set and the field instance already exists, | ||
| // run validation without permanently marking the field as touched | ||
| if (options?.avoidTouch) { | ||
| const fieldInstance = this.fieldInfo[nestedField]?.instance | ||
| if (fieldInstance) { | ||
| const wasTouched = fieldInstance.state.meta.isTouched | ||
| if (!wasTouched) { | ||
| // Temporarily touch the field so validation runs, then restore | ||
| fieldInstance.setMeta((prev) => ({ | ||
| ...prev, | ||
| isTouched: true, | ||
| })) | ||
| } | ||
| const result = await fieldInstance.validate(cause) | ||
| if (!wasTouched) { | ||
| fieldInstance.setMeta((prev) => ({ | ||
| ...prev, | ||
| isTouched: false, | ||
| })) | ||
| } | ||
| return result | ||
| } | ||
| } | ||
| return this.validateField(nestedField, cause) | ||
| }), |
There was a problem hiding this comment.
Guard restoration with try/finally, and note the transient-state exposure.
Two concerns with the temporary-touch approach:
- No error safety. If
fieldInstance.validate(cause)throws (sync throw before returning a Promise, or a rejected Promise returned synchronously before theawait), the secondsetMetanever runs and the field is left permanentlyisTouched: true— reintroducing the exact bug this PR fixes. Wrap intry/finally. - Observable transient state. Between the two
setMetacalls, the derived stores (fieldMetaDerived,store.isTouched, devtools broadcast viathrottleFormState) momentarily reflectisTouched: truefor every untouched sibling. With N shifted siblings all toggling in parallel, subscribers/listeners can observe a spurious "all touched" window. Also, if the user genuinely touches the field during theawait, the restoration will clobber their touch back tofalse.
The try/finally is a strict correctness fix and should be applied. The transient-state issue is the main reason the originally suggested refactor (an internal { touch: false } path on validateField that skips the isTouched check entirely) is cleaner — worth reconsidering as a follow-up.
🛡️ Proposed fix: ensure restoration on error paths
if (options?.avoidTouch) {
const fieldInstance = this.fieldInfo[nestedField]?.instance
if (fieldInstance) {
const wasTouched = fieldInstance.state.meta.isTouched
if (!wasTouched) {
// Temporarily touch the field so validation runs, then restore
fieldInstance.setMeta((prev) => ({
...prev,
isTouched: true,
}))
}
- const result = await fieldInstance.validate(cause)
- if (!wasTouched) {
- fieldInstance.setMeta((prev) => ({
- ...prev,
- isTouched: false,
- }))
- }
- return result
+ try {
+ return await fieldInstance.validate(cause)
+ } finally {
+ if (!wasTouched) {
+ fieldInstance.setMeta((prev) => ({
+ ...prev,
+ isTouched: false,
+ }))
+ }
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/form-core/src/FormApi.ts` around lines 1609 - 1634, The
temporary-touch logic inside the Promise.resolve block when options?.avoidTouch
is true should ensure the original isTouched is always restored even if
fieldInstance.validate(cause) throws or rejects: wrap the validate call and the
second setMeta restoration in a try/finally so that after temporarily setting
fieldInstance.setMeta(... isTouched: true) you always run the restore
setMeta(...) in the finally block; keep the early-return of the validate result,
and otherwise fall back to this.validateField(nestedField, cause). Reference the
Promise.resolve(...) block, options?.avoidTouch, fieldInstance,
fieldInstance.validate(cause), and fieldInstance.setMeta(...) when making the
change.
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.
What changed
In
validateArrayFieldsStartingFrom, fields with an existing instance now callfieldInstance.validate(cause)directly instead of going throughthis.validateField(). This avoids the auto-touch side-effect invalidateFieldthat marks fields as touched even when the user never interacted with them.Why
Calling
form.removeValue(index)on an array field was flippingisTouchedtotrueon every sibling at index ≥ removed index. The bug lives invalidateFieldwhich auto-touches any field that has an instance but isn't yet touched.How to reproduce
form.removeValue(0)without interacting with other fieldsisTouchedbecomestrueon all shifted siblingsCloses #2131
Summary by CodeRabbit