fix(form-core): reset stale validation counter when shifting array meta - #2247
fix(form-core): reset stale validation counter when shifting array meta#2247xianjianlf2 wants to merge 3 commits into
Conversation
When an array item is removed (or inserted/moved) while an async validator is in flight, `shiftMeta` copied the source field's transient `_pendingValidationsCount`/`isValidating` down to the shifted index. The pending promise is owned by the original field instance and settles its counter against the original index, so the shifted index's counter is orphaned and never decremented, leaving `isFieldsValidating` stuck `true`. Reset the transient validation state when shifting meta, and guard `endValidation` so a validation that resolves after its field was removed no longer resurrects deleted meta. Closes TanStack#2234
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change prevents stale async-validation state after array values are removed or reordered by guarding inactive field validation completion and resetting transient metadata during shifts. Tests cover removal, move, and swap scenarios, with a patch Changesets entry. ChangesArray async-validation cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/form-core/src/metaHelper.ts (1)
100-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset async-validation state in
handleArrayMove/handleArraySwap. ReapplyingfromMetaand swapping meta verbatim carriesisValidatingand_pendingValidationsCountto the new index, so an in-flight validation can stay stuck on the wrong field. MirrorshiftMeta’s reset here and add a move/swap regression test.🤖 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/metaHelper.ts` around lines 100 - 116, Reset async-validation state when restoring metadata in handleArrayMove and handleArraySwap, matching shiftMeta’s reset behavior instead of copying isValidating and _pendingValidationsCount unchanged. Update the metadata transfer logic around fromMeta and swapped metadata, then add regression coverage verifying moved and swapped fields do not retain in-flight validation state.
🤖 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.
Outside diff comments:
In `@packages/form-core/src/metaHelper.ts`:
- Around line 100-116: Reset async-validation state when restoring metadata in
handleArrayMove and handleArraySwap, matching shiftMeta’s reset behavior instead
of copying isValidating and _pendingValidationsCount unchanged. Update the
metadata transfer logic around fromMeta and swapped metadata, then add
regression coverage verifying moved and swapped fields do not retain in-flight
validation state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 48e8bfdf-c589-40d8-979e-008a9b920059
📒 Files selected for processing (4)
.changeset/fix-array-removevalue-isvalidating-stuck.mdpackages/form-core/src/FieldApi.tspackages/form-core/src/metaHelper.tspackages/form-core/tests/FieldApi.spec.ts
|
Addressed the CodeRabbit follow-up in 8488fc3. Move and swap now use the same transient async-validation reset as shifted array meta, so Local checks:
|
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/metaHelper.ts`:
- Around line 113-116: Preserve validation metadata for no-op operations by
returning early when fromIndex === toIndex in moveValue at
packages/form-core/src/metaHelper.ts:113-116, and when index === secondIndex in
swapValues at packages/form-core/src/metaHelper.ts:159-162, before resetting
field metadata. Add regression coverage for same-index moveValue and swapValues
calls with dontValidate enabled to verify pending validation remains intact.
🪄 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: 406bd3eb-5e14-4388-a433-7a8336813edd
📒 Files selected for processing (2)
packages/form-core/src/metaHelper.tspackages/form-core/tests/FieldApi.spec.ts
|
Addressed the same-index move/swap follow-up in What changed:
Local check: pnpm --filter @tanstack/form-core exec vitest run tests/FieldApi.spec.tsResult: 1 file / 110 tests passed, no type errors. |
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
Checked how this.getInfo().instance !== this is used elsewhere in FieldApi.ts before judging this guard, and it's not a new idea being introduced here, the async validator resolution path around line 1554 already bails out of writing setMeta with that exact same check when a field's identity has been superseded. So endValidation() picking up the same guard is consistent with how the file already treats "this instance no longer owns this name" elsewhere, rather than a one-off patch.
What makes this actually close the bug rather than just move it is that endValidation() runs unconditionally after Promise.all(validatesPromises) in validateAsync, regardless of that existing instance check further up (which only guards the error-writing setMeta, not the counter decrement). So before this fix, even in the path that correctly skipped writing a stale error, the pending-validation counter on whatever now occupies that field name would still get decremented by an instance that no longer owns it, which is exactly how isFieldsValidating could end up permanently wrong after removeValue shifts indices.
The resetTransientValidationMeta addition in metaHelper.ts is the other half of the same problem: when array mutation copies old meta onto a new key, carrying over isValidating/_pendingValidationsCount would leave a counter at the new key that nothing will ever decrement, since the instance that started that validation is bailing out via the guard above instead of settling it. Resetting it at copy time and letting the shift naturally re-trigger validation on the moved field is the right call.
The fromIndex === toIndex / index === secondIndex early returns are a nice touch beyond the core fix, and the test for the same-index case (should preserve transient async validation state when moving array field meta to the same index) is what actually proves the reset only fires on a real move and not on a no-op call, which is easy to get backwards.
The main regression test reconstructs the real scenario end to end, an async validator on a listened-to field, in flight, then removing a different array index while it's pending, then resolving it and checking the counter settles at zero rather than asserting some internal method got called. That's the right level to test this at since the bug only shows up through that exact interleaving.
2 participants
🎯 Changes
Fixes #2234. When an array item is removed (or inserted/moved) while an async validator is in flight,
shiftMetacopied the source field's transient_pendingValidationsCount/isValidatingdown to the shifted index. The pending promise is owned by the original field instance and settles its counter against the original index, so the shifted index's counter is orphaned and never decremented — leavingform.state.isFieldsValidatingstucktrueand blocking submission.shiftMetanow resets the transient validation state (isValidating,_pendingValidationsCount) when shifting meta between indices. Array mutations already re-trigger validation on the shifted fields, so this is safe.endValidationbails out if the field's instance is no longer registered, so a validation that resolves after its field was removed no longer resurrects deleted meta (previously threw aTypeError).✅ Checklist
🚀 Release Impact
Summary by CodeRabbit