fix(form-core): preserve reset(newValues) when update() is called with stale options#2218
Open
LeSingh1 wants to merge 1 commit into
Open
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
Comment on lines
+1760
to
+1773
| // 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.
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.
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