fix(form-core): preserve reset(newValues) when update() is called with stale options - #2218
Conversation
…h stale options When reset(values) is called inside onSubmit, the new values were being overwritten on the next render cycle because useIsomorphicLayoutEffect calls formApi.update(opts) with the original render-closure options. update() compared options.defaultValues (original) to this.options.defaultValues (set by reset), found a difference, and reset the form state back to the original defaults. Fix: add a private _defaultValuesOverridden flag that is set by reset() whenever new explicit values are provided. update() now checks this flag and, when set, skips the shouldUpdateValues logic and preserves the defaultValues that reset() installed. The flag is cleared after one update() cycle so subsequent prop-driven defaultValues changes still work. Fixes TanStack#1681
📝 WalkthroughWalkthrough
ChangesReset/update race condition fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
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
🧹 Nitpick comments (1)
packages/form-core/tests/FormApi.spec.ts (1)
147-173: ⚡ Quick winAdd one assertion path for
defaultStatechanges during the override cycle.Given the new one-cycle guard in
update(), it’s worth adding a companion case where staledefaultValuesare ignored but a realdefaultStatedelta still applies. That locks in the intended contract and prevents regressions around this race fix.🤖 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` around lines 147 - 173, Add an assertion path to the test that verifies legitimate defaultState changes are still applied during the update cycle. After the final update(originalOptions) call, add a scenario where you update the form with new defaultState values and assert that those changes are properly applied to form.state, ensuring that the guard preventing stale defaultValues from overwriting reset values does not inadvertently block valid defaultState updates. This locks in the contract that the one-cycle guard in the update() method only blocks stale defaultValues while still allowing real defaultState deltas to apply.
🤖 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 1760-1773: The early return in the `_defaultValuesOverridden`
block within the `update()` method prevents `shouldUpdateState` from being
evaluated, which causes changes to `options.defaultState` to be dropped in that
cycle. Instead of returning early after handling the defaultValues override,
continue processing the update by allowing the `shouldUpdateState` evaluation to
proceed. Merge the override logic so that the old defaultValues are preserved
while still permitting synchronization of defaultState changes in the same
cycle.
---
Nitpick comments:
In `@packages/form-core/tests/FormApi.spec.ts`:
- Around line 147-173: Add an assertion path to the test that verifies
legitimate defaultState changes are still applied during the update cycle. After
the final update(originalOptions) call, add a scenario where you update the form
with new defaultState values and assert that those changes are properly applied
to form.state, ensuring that the guard preventing stale defaultValues from
overwriting reset values does not inadvertently block valid defaultState
updates. This locks in the contract that the one-cycle guard in the update()
method only blocks stale defaultValues while still allowing real defaultState
deltas to apply.
🪄 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: 56b6b81d-b98f-4195-bf44-b4c411ddb596
📒 Files selected for processing (2)
packages/form-core/src/FormApi.tspackages/form-core/tests/FormApi.spec.ts
| // If reset(newValues) was called, the incoming `options` still carries the | ||
| // original (pre-reset) defaultValues from the render closure. We must not | ||
| // let those stale values overwrite what reset() set, so we skip the | ||
| // shouldUpdateValues branch and clear the flag for the next render. | ||
| if (this._defaultValuesOverridden) { | ||
| this._defaultValuesOverridden = false | ||
| // Keep the defaultValues that reset() stored on this.options so that | ||
| // future renders compare against them correctly. | ||
| this.options = { | ||
| ...options, | ||
| defaultValues: oldOptions.defaultValues, | ||
| } | ||
| return | ||
| } |
There was a problem hiding this comment.
Line 1764: early return unintentionally suppresses defaultState synchronization.
When _defaultValuesOverridden is set, update() returns before evaluating shouldUpdateState, so a real options.defaultState change in that cycle can be silently dropped.
💡 Suggested fix
- if (this._defaultValuesOverridden) {
- this._defaultValuesOverridden = false
- // Keep the defaultValues that reset() stored on this.options so that
- // future renders compare against them correctly.
- this.options = {
- ...options,
- defaultValues: oldOptions.defaultValues,
- }
- return
- }
+ let nextOptions = options
+ if (this._defaultValuesOverridden) {
+ this._defaultValuesOverridden = false
+ // Keep defaultValues written by reset(), but still allow normal update()
+ // processing (eg defaultState sync) in this cycle.
+ nextOptions = {
+ ...options,
+ defaultValues: oldOptions.defaultValues,
+ }
+ this.options = nextOptions
+ }
- const shouldUpdateValues =
- options.defaultValues &&
- !evaluate(options.defaultValues, oldOptions.defaultValues) &&
+ const shouldUpdateValues =
+ nextOptions.defaultValues &&
+ !evaluate(nextOptions.defaultValues, oldOptions.defaultValues) &&
!this.state.isTouched
const shouldUpdateState =
- !evaluate(options.defaultState, oldOptions.defaultState) &&
+ !evaluate(nextOptions.defaultState, oldOptions.defaultState) &&
!this.state.isTouched
@@
- shouldUpdateState ? options.defaultState : {},
+ shouldUpdateState ? nextOptions.defaultState : {},
shouldUpdateValues
? {
- values: options.defaultValues,
+ values: nextOptions.defaultValues,
}
: {},🤖 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 1760 - 1773, The early return
in the `_defaultValuesOverridden` block within the `update()` method prevents
`shouldUpdateState` from being evaluated, which causes changes to
`options.defaultState` to be dropped in that cycle. Instead of returning early
after handling the defaultValues override, continue processing the update by
allowing the `shouldUpdateState` evaluation to proceed. Merge the override logic
so that the old defaultValues are preserved while still permitting
synchronization of defaultState changes in the same cycle.
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
The direction is right, but the one-shot flag reverts if update() fires twice in a row with the same stale props before a genuinely new set arrives, which is a realistic case (a parent re-rendering for an unrelated reason re-runs the child's layout effect with the same props reference, or StrictMode double-invoking it).
Reimplemented both this PR's logic and #2235's (which targets the same #1681, comparing against the previous update's props instead of a one-shot flag) side by side and ran the exact scenario, reset, then two consecutive update() calls with unchanged stale props:
[PR#2218 (flag)] after reset: { name: 'new-default' }
[PR#2218 (flag)] after update #1 (stale): { name: 'new-default' }
[PR#2218 (flag)] after update #2 (same stale props again): { name: 'original' }
[PR#2218 (flag)] reset value survived double stale update: false
[PR#2235 (tracking)] after reset: { name: 'new-default' }
[PR#2235 (tracking)] after update #1 (stale): { name: 'new-default' }
[PR#2235 (tracking)] after update #2 (same stale props again): { name: 'new-default' }
[PR#2235 (tracking)] reset value survived double stale update: true
The flag gets consumed by the first update() call after reset(), so by the second consecutive call with the same (unchanged) props, _defaultValuesOverridden is already false and update() falls through to the normal shouldUpdateValues comparison, comparing the stale incoming props against the defaultValues that reset() set, which reads as a real change and reverts.
#2235 doesn't have this gap because it tracks what the previous update() call actually saw, not a one-shot signal, so a repeated identical update() call keeps comparing against the same baseline instead of consuming a flag. It also doesn't need the early return this PR adds (which skips the rest of update(), including shouldUpdateState, on the cycle it fires), since it folds the reset-preservation into the same this.options assignment that runs either way.
Since #2235 is already open against the same issue and (at least in this scenario) doesn't have the gap, might be worth comparing the two directly before merging either.
2 participants
Summary
Fixes #1681 —
form.reset(newValues)called insideonSubmitis ignored and the form reverts to its original default values.Root cause
reset(values)(with explicit new values) does two things:this.options.defaultValues = valuesin-placethis.baseStore.setState(...)with the new valuesAfter the
onSubmitpromise resolves, React re-renders (becauseisSubmittingflipped back tofalse). The framework then callsformApi.update(opts)insideuseIsomorphicLayoutEffect— whereoptsis the original options object captured in the render closure, still holding the pre-resetdefaultValues.Inside
update():oldOptions = this.options→ has the new defaultValues (set by reset)this.options = options→ overwrites back to original defaultValuesoptions.defaultValues !== oldOptions.defaultValues→true!this.state.isTouched→true(reset cleared touched state)shouldUpdateValues = true→ form values reset to original defaults ❌Fix
Add a private
_defaultValuesOverriddenflag onFormApi. Whenreset(values)is called with explicit values (and!keepDefaultValues), set the flag totrue. Inupdate(), if the flag is set, skip theshouldUpdateValuesbranch and preserve thedefaultValuesthatreset()stored onthis.options. The flag is cleared after oneupdate()cycle so subsequent prop-drivendefaultValueschanges continue to work correctly.Test plan
packages/form-core/tests/FormApi.spec.tsthat directly simulates the race condition: callsreset(newValues)thenupdate(originalOpts)and asserts that values are not revertedform should reset default value when resetting in onSubmit(both core and react-form suites) continues to pass@tanstack/form-coretests pass@tanstack/react-formtests passSummary by CodeRabbit