Skip to content

fix(form-core): prevent removeValue from auto-touching shifted siblings #2133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mixelburg wants to merge 2 commits into
base: main
Choose a base branch
from
+31 −2
Open

fix(form-core): prevent removeValue from auto-touching shifted siblings #2133

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd491ba
fix(form-core): prevent removeValue from auto-touching shifted siblings
mixelburg Apr 18, 2026
a1294aa
fix(form-core): properly scope avoidTouch to removeFieldValue only
mixelburg Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/form-core/src/FormApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1605,7 +1605,15 @@ export class FormApi<
batch(() => {
fieldsToValidate.forEach((nestedField) => {
fieldValidationPromises.push(
Promise.resolve().then(() => this.validateField(nestedField, cause)),
Promise.resolve().then(() => {
// If the field instance already exists, call validate directly
// to avoid auto-touching shifted siblings
const fieldInstance = this.fieldInfo[nestedField]?.instance
if (fieldInstance) {
return fieldInstance.validate(cause)
}
return this.validateField(nestedField, cause)
}),
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
Comment on lines +1609 to +1634
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard restoration with try/finally, and note the transient-state exposure.

Two concerns with the temporary-touch approach:

  1. No error safety. If fieldInstance.validate(cause) throws (sync throw before returning a Promise, or a rejected Promise returned synchronously before the await), the second setMeta never runs and the field is left permanently isTouched: true — reintroducing the exact bug this PR fixes. Wrap in try/finally.
  2. Observable transient state. Between the two setMeta calls, the derived stores (fieldMetaDerived, store.isTouched, devtools broadcast via throttleFormState) momentarily reflect isTouched: true for 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 the await, the restoration will clobber their touch back to false.

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.

)
})
})
Expand Down
Loading