fix(FieldApi): fix race condition when using onChangeListenTo#2143
Open
Pascalmh wants to merge 4 commits intoTanStack:mainfrom
Open
fix(FieldApi): fix race condition when using onChangeListenTo#2143Pascalmh wants to merge 4 commits intoTanStack:mainfrom
Pascalmh wants to merge 4 commits intoTanStack:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAsync validation now tracks per-field active async runs with a counter, isolating a field’s own async validators from linked fields’ validators. Initialization and defaults include the counter; tests simulate rapid setValue calls to ensure no linked field remains stuck validating. ChangesPer-field async validation counter and isolated linked-field tracking
Sequence Diagram(s)sequenceDiagram
participant UI as rgba(66,133,244,0.5) UI
participant Field as rgba(219,68,55,0.5) FieldApi (street)
participant Linked as rgba(15,157,88,0.5) Linked FieldApis (houseNo, zipCode, city)
participant Validator as rgba(142,36,170,0.5) Async Validator
participant Form as rgba(255,193,7,0.5) Form State
UI->>Field: rapid setValue calls (simulate autofill)
Field->>Field: determine hasAsyncValidators (self) and linkedFieldsWithAsyncValidators
Field->>Field: startValidation() for self if needed
Field->>Linked: startValidation() only for linkedFieldsWithAsyncValidators
Field->>Validator: invoke debounced async validation
Validator-->>Field: return result
Field->>Field: endValidation() for self if finished
Field->>Linked: endValidation() for linkedFieldsWithAsyncValidators
Field->>Form: update isFieldsValidating and validity flags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 1m 39s | View ↗ |
nx run-many --target=build --exclude=examples/** |
✅ Succeeded | 33s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-05-05 08:09:47 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2143 +/- ##
==========================================
- Coverage 90.35% 90.27% -0.08%
==========================================
Files 38 49 +11
Lines 1752 2047 +295
Branches 444 533 +89
==========================================
+ Hits 1583 1848 +265
- Misses 149 179 +30
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/form-core/src/FieldApi.ts (1)
1868-1872: ⚡ Quick win
isValidatingis set/cleared on all linked fields, including those with no async validators.
hasLinkedAsyncValidatorsgates the loop on any linked field having an async validator, but the loop body then marks every field inlinkedFields. A linked field that only has sync validators (or no validators) gets a spuriousisValidating: true → falseflicker, which can cause unnecessary UI loading-indicator renders.Because each
linkedFieldValidatesentry already carries a.fieldreference (attached at line 1836), the target set is cheap to compute.♻️ Proposed refactor — iterate only over fields that have actual async validators
+ const linkedFieldsWithAsyncValidators = new Set( + linkedFieldValidates.filter((v) => v.validate).map((v) => v.field), + ) const hasLinkedAsyncValidators = linkedFieldValidates.some( (v) => v.validate, ) if (hasLinkedAsyncValidators) { - for (const linkedField of linkedFields) { + for (const linkedField of linkedFieldsWithAsyncValidators) { linkedField.setMeta((prev) => ({ ...prev, isValidating: true })) } }if (hasLinkedAsyncValidators) { - for (const linkedField of linkedFields) { + for (const linkedField of linkedFieldsWithAsyncValidators) { linkedField.setMeta((prev) => ({ ...prev, isValidating: false })) } }Also applies to: 1988-1992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/form-core/src/FieldApi.ts` around lines 1868 - 1872, hasLinkedAsyncValidators currently gates updating linkedFields but then sets isValidating on every linkedField; change the loops so they only iterate over the actual fields that have async validators by using the linkedFieldValidates entries (each contains a .field) instead of linkedFields. Specifically, replace the for (const linkedField of linkedFields) { linkedField.setMeta(prev => ({...prev, isValidating: true})) } with a loop over linkedFieldValidates.map(e => e.field) (or filter linkedFields by presence in linkedFieldValidates) and do the same change for the corresponding clearing loop (the block around the other occurrence noted at ~1988-1992) so only fields with async validators receive the isValidating updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1868-1872: hasLinkedAsyncValidators currently gates updating
linkedFields but then sets isValidating on every linkedField; change the loops
so they only iterate over the actual fields that have async validators by using
the linkedFieldValidates entries (each contains a .field) instead of
linkedFields. Specifically, replace the for (const linkedField of linkedFields)
{ linkedField.setMeta(prev => ({...prev, isValidating: true})) } with a loop
over linkedFieldValidates.map(e => e.field) (or filter linkedFields by presence
in linkedFieldValidates) and do the same change for the corresponding clearing
loop (the block around the other occurrence noted at ~1988-1992) so only fields
with async validators receive the isValidating updates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79e4efed-e8e0-40a1-b9f4-da213214b504
📒 Files selected for processing (2)
packages/form-core/src/FieldApi.tspackages/form-core/tests/FormApi.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/form-core/tests/FormApi.spec.ts
Pascalmh
force-pushed
the
bugfix/onChangeListenTo
branch
from
aaf5a95 to
ea9d5fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/form-core/src/FieldApi.ts (1)
1858-1866: ⚡ Quick winSimplify
linkedFieldsWithAsyncValidators— the.some()guard is redundant.The ternary short-circuits on
.some()to avoid allocating an empty array, butfilter(...).map(...)on a collection that produces no results already returns[]. The current code iterateslinkedFieldValidatestwice unnecessarily.♻️ Proposed simplification
- const linkedFieldsWithAsyncValidators = linkedFieldValidates.some( - (v) => v.validate, - ) - ? Array.from( - new Set( - linkedFieldValidates.filter((v) => v.validate).map((v) => v.field), - ), - ) - : [] + const linkedFieldsWithAsyncValidators = Array.from( + new Set( + linkedFieldValidates.filter((v) => v.validate).map((v) => v.field), + ), + )🤖 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/FieldApi.ts` around lines 1858 - 1866, The current computation of linkedFieldsWithAsyncValidators redundantly calls linkedFieldValidates twice by using .some() as a guard; replace that ternary with a single expression that always builds the unique list (e.g., Array.from(new Set(linkedFieldValidates.filter(v => v.validate).map(v => v.field)))) so you only iterate linkedFieldValidates once and get [] when there are no matches; update the assignment where linkedFieldsWithAsyncValidators is declared in FieldApi.ts accordingly.
🤖 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/FieldApi.ts`:
- Around line 1874-1876: The linked-field race occurs because validateAsync for
a field clears another field's meta.isValidating when its own linked validations
finish, even if that other field has its own in-flight async validation; fix by
tracking in-flight validation owners per field (e.g., add a Map/Set like
validatingOwners or a numeric validateCount stored on field meta) and update
validateAsync and linked validation paths to increment the owner before marking
isValidating true and decrement on completion/abort, only setting
meta.isValidating = false when the owner count/Set for that field reaches zero;
update the code paths around linkedFieldsWithAsyncValidators, linkedPromises
resolution, the abort handling, and the blocks that currently call
linkedField.setMeta(... isValidating: false ...) so they use the new owner-aware
logic (reference symbols: validateAsync, linkedFieldsWithAsyncValidators,
linkedPromises, setMeta, isValidating, and the computed
form.state.isFieldsValidating).
---
Nitpick comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1858-1866: The current computation of
linkedFieldsWithAsyncValidators redundantly calls linkedFieldValidates twice by
using .some() as a guard; replace that ternary with a single expression that
always builds the unique list (e.g., Array.from(new
Set(linkedFieldValidates.filter(v => v.validate).map(v => v.field)))) so you
only iterate linkedFieldValidates once and get [] when there are no matches;
update the assignment where linkedFieldsWithAsyncValidators is declared in
FieldApi.ts accordingly.
🪄 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: 3a119c7c-c7af-4b83-b380-5e47d3f11a67
📒 Files selected for processing (2)
packages/form-core/src/FieldApi.tspackages/form-core/tests/FormApi.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/form-core/tests/FormApi.spec.ts
Pascalmh
force-pushed
the
bugfix/onChangeListenTo
branch
from
1e22a04 to
8551da0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/form-core/tests/FormApi.spec.ts (1)
2995-2995: ⚡ Quick win
vi.useRealTimers()should be in anafterEach(ortry/finally) rather than inlineAs written, if
vi.runAllTimersAsync()on line 3039 throws unexpectedly, thevi.useRealTimers()on line 3053 is never reached, leaving fake timers active for subsequent tests. Because all assertions areexpect.soft(non-throwing), this risk is limited to an exception fromvi.runAllTimersAsync()itself — but that's a real, non-trivial surface area.The conventional fix is either:
♻️ Option A — `afterEach` (preferred, aligns with Vitest idioms)
+ afterEach(() => { + vi.useRealTimers() + }) + it('should not leave linked fields stuck in isValidating when multiple setValue calls trigger concurrent async validation', async () => { vi.useFakeTimers() // ... - vi.useRealTimers() })♻️ Option B — `try/finally` (if a scoped `afterEach` is not desirable)
it('should not leave linked fields stuck in isValidating ...', async () => { vi.useFakeTimers() + try { // ... body ... await vi.runAllTimersAsync() expect.soft(validationFn).toHaveBeenCalledTimes(1) // ... - vi.useRealTimers() + } finally { + vi.useRealTimers() + } })Also applies to: 3053-3053
🤖 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/tests/FormApi.spec.ts` at line 2995, The test enables fake timers with vi.useFakeTimers() but restores them inline with vi.useRealTimers(), risking leaked fake timers if vi.runAllTimersAsync() throws; update the test around vi.useFakeTimers()/vi.runAllTimersAsync()/vi.useRealTimers() to guarantee restoration by either moving vi.useRealTimers() into an afterEach hook that always runs, or wrap the block in a try/finally (call vi.useRealTimers() in finally) so vi.useRealTimers() is always executed; search for vi.useFakeTimers(), vi.runAllTimersAsync(), and vi.useRealTimers() in the test to apply the fix.
🤖 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.
Nitpick comments:
In `@packages/form-core/tests/FormApi.spec.ts`:
- Line 2995: The test enables fake timers with vi.useFakeTimers() but restores
them inline with vi.useRealTimers(), risking leaked fake timers if
vi.runAllTimersAsync() throws; update the test around
vi.useFakeTimers()/vi.runAllTimersAsync()/vi.useRealTimers() to guarantee
restoration by either moving vi.useRealTimers() into an afterEach hook that
always runs, or wrap the block in a try/finally (call vi.useRealTimers() in
finally) so vi.useRealTimers() is always executed; search for
vi.useFakeTimers(), vi.runAllTimersAsync(), and vi.useRealTimers() in the test
to apply the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55fa2bb8-fff8-457f-9d70-f86264a0e5f0
📒 Files selected for processing (3)
.changeset/true-impalas-behave.mdpackages/form-core/src/FieldApi.tspackages/form-core/tests/FormApi.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/true-impalas-behave.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/form-core/src/FieldApi.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/form-core/src/FieldApi.ts (1)
1906-1912: ⚡ Quick winBatch the per-field
startValidation/endValidationcalls to avoid N+1 store updates.Each
startValidation/endValidationcall goes throughsetMeta→form.setFieldMeta→baseStore.setState, which triggersfieldMetaDerivedandstorerecomputation. With several linked fields, the loops at lines 1910–1912 and 2028–2030 trigger one recomputation per field per phase. Wrapping each phase inbatch()(already imported on line 1) collapses them into a single store update.♻️ Proposed batching
- if (hasAsyncValidators) { - this.startValidation() - } - - for (const linkedField of linkedFieldsWithAsyncValidators) { - linkedField.startValidation() - } + batch(() => { + if (hasAsyncValidators) { + this.startValidation() + } + for (const linkedField of linkedFieldsWithAsyncValidators) { + linkedField.startValidation() + } + })- // Only reset isValidating if we set it to true earlier - if (hasAsyncValidators) { - this.endValidation() - } - - for (const linkedField of linkedFieldsWithAsyncValidators) { - linkedField.endValidation() - } + batch(() => { + // Only reset isValidating if we set it to true earlier + if (hasAsyncValidators) { + this.endValidation() + } + for (const linkedField of linkedFieldsWithAsyncValidators) { + linkedField.endValidation() + } + })Also applies to: 2024-2030
🤖 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/FieldApi.ts` around lines 1906 - 1912, The per-field calls to startValidation and endValidation cause N+1 store updates; wrap the blocks that call this.startValidation() (guarded by hasAsyncValidators) and the for-loop over linkedFieldsWithAsyncValidators, and likewise the matching endValidation block (the loops that call this.endValidation() and linkedField.endValidation()), inside batch(...) so the multiple setMeta → form.setFieldMeta → baseStore.setState updates collapse into a single batched update; locate the calls by the symbols startValidation, endValidation, and linkedFieldsWithAsyncValidators and replace the separate calls with a single batch call that executes them all.
🤖 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/FieldApi.ts`:
- Around line 1800-1830: endValidation unconditionally decrements
validationCount causing it to go negative and can leave isValidating stuck;
change endValidation (and post-Promise.all callers) to defensively no-op when
prev.validationCount === 0 (or clamp newCount = Math.max(0, prev.validationCount
- 1)) and only set isValidating to false when newCount === 0 and
prev.isValidating is true; additionally short-circuit stale validation
completions by checking the field instance (mirror the existing
getInfo().instance !== field guard) before calling endValidation so
unmount/reset paths (unmount, form.resetField, setMeta(defaultFieldMeta)) cannot
mutate a freshly reset meta.
---
Nitpick comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1906-1912: The per-field calls to startValidation and
endValidation cause N+1 store updates; wrap the blocks that call
this.startValidation() (guarded by hasAsyncValidators) and the for-loop over
linkedFieldsWithAsyncValidators, and likewise the matching endValidation block
(the loops that call this.endValidation() and linkedField.endValidation()),
inside batch(...) so the multiple setMeta → form.setFieldMeta →
baseStore.setState updates collapse into a single batched update; locate the
calls by the symbols startValidation, endValidation, and
linkedFieldsWithAsyncValidators and replace the separate calls with a single
batch call that executes them all.
🪄 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: b4e4f882-5129-486d-971d-16ddd69b3c47
📒 Files selected for processing (3)
packages/form-core/src/FieldApi.tspackages/form-core/src/FormApi.tspackages/form-core/src/metaHelper.ts
Pascalmh
force-pushed
the
bugfix/onChangeListenTo
branch
from
5f7e2ca to
c0fd89f
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Pascalmh
changed the title
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.
🎯 Changes
ToDo:
isValidatingremianstruewhen multiple fields are set simultaniasly in while usingonChangeListenToon multiple fields and runningonChangeAsyncchecks✅ Checklist
pnpm test:pr.🚀 Release Impact
Summary by CodeRabbit