Open
Conversation
🦋 Changeset detectedLatest commit: 8b41487 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Fixes the issue with field unmount in core.
|
View your CI Pipeline Execution ↗ for commit 8b41487
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2068 +/- ##
==========================================
- Coverage 90.35% 90.13% -0.22%
==========================================
Files 38 49 +11
Lines 1752 2038 +286
Branches 444 532 +88
==========================================
+ Hits 1583 1837 +254
- Misses 149 181 +32
Partials 20 20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds an optional Form option Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Form as FormApi
participant Field as FieldApi
participant Val as ValidationSystem
App->>Form: create(formOptions with cleanupFieldsOnUnmount=true)
App->>Field: mount field
Field->>Form: register field instance & metadata
Field->>Val: start validations / set timeouts / listeners
Field-->>App: return cleanup function
Note over Val: validations/timeouts in-flight
App->>Field: call cleanup() on unmount
Field->>Val: abort controllers & cancel timeouts/listeners
Val-->>Val: stop async callbacks
Field->>Form: reset field meta, clear instance reference
Field-->>App: cleanup complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1361-1375: The abort loop currently runs before checking whether
this FieldApi instance is still the active one, which can cancel validations for
a newer remounted instance; move the instance check so it runs before touching
shared state (i.e., check fieldInfo.instance !== this and return early), or
alternatively inside the loop verify fieldInfo.instance === this before calling
validationMeta?.lastAbortController.abort() and clearing entries; update the
code paths around fieldInfo, validationMetaMap, lastAbortController, and the
instance guard to ensure only the active FieldApi instance aborts and clears the
shared validationMetaMap.
- Around line 1377-1383: The current reset in FieldApi using
this.form.baseStore.setState(...) replaces prev.fieldMetaBase[this.name] with
defaultFieldMeta and thus wipes preserved state on unmount/remount; update the
setter to merge with any existing meta for this.name instead of overwriting so
preserved flags/values survive (e.g., read existing =
prev.fieldMetaBase?.[this.name], then set fieldMetaBase[this.name] = {
...defaultFieldMeta, ...existing } or selectively preserve keys like
touched/value/defaultValueSeeded) so the logic in FieldApi that reseeds
options.defaultValue (referenced by the FieldApi methods around lines that
handle defaultValue reseeding) no longer loses user-entered values on remount.
In `@packages/react-form/tests/useField.test.tsx`:
- Around line 422-423: The test asserts onSubmit immediately after clicking
submit but form.handleSubmit() executes asynchronously; update the test to wait
for the async submit path to complete before asserting by awaiting a wait helper
that checks onSubmit (e.g., use waitFor or another async wait) so the assertion
verifies that onSubmit was called after form._handleSubmit()/form.handleSubmit()
finishes; reference the submitButton click and the onSubmit mock in the updated
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2c69fac-1e7b-4d00-b470-721c4ba1c718
📒 Files selected for processing (5)
.changeset/red-hats-jam.mdpackages/form-core/src/FieldApi.tspackages/form-core/src/FormApi.tspackages/form-core/tests/FieldApi.spec.tspackages/react-form/tests/useField.test.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/form-core/src/FieldApi.ts`:
- Around line 1364-1375: The teardown skips aborting async work started by the
older FieldApi instance because it only avoids touching shared controllers when
a newer instance is mounted; fix by either tracking abort controllers per
FieldApi instance or by ignoring async completions when the instance no longer
matches. Concretely: change the validation logic so each FieldApi stores its own
controllers (e.g., a per-instance map on this) instead of reusing
fieldInfo.validationMetaMap.lastAbortController, and abort only those on
teardown; or add a guard in the async validator completion path (before calling
field.setMeta(...) / inside FieldApi's async result handling) that checks
field.getInfo().instance === this and returns early if it differs. Ensure you
reference validationMetaMap, lastAbortController, FieldApi, setMeta and getInfo
when locating the relevant code to update.
- Around line 1330-1359: Currently async validation/listener
timeouts/controllers are attached to the active instance (this) even when the
async work targets a different field, so the target field cannot cancel them on
unmount; update the code that schedules linked-field async work (the
validateAsync caller that writes to getInfo().validationMetaMap and
this.timeoutIds.validations / this.timeoutIds.listeners) to store ownership on
the target field instead of this (i.e., use the target field's timeoutIds and
validationMetaMap entry keys), and update the teardown returned by the FieldApi
cleanup to clear any timeouts/controllers stored on the field itself (iterate
and clear entries on field.timeoutIds.validations/listeners/formListeners and
remove related validationMetaMap entries) so the target field’s unmount cancels
its own async work. Ensure you reference and update validateAsync,
getInfo().validationMetaMap, and timeoutIds.validations/listeners/formListeners
usage sites so ownership is moved to the field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39798198-5e48-4350-8160-450fd6a0e1ea
📒 Files selected for processing (3)
packages/form-core/src/FieldApi.tspackages/form-core/tests/FieldApi.spec.tspackages/react-form/tests/useField.test.tsx
Comment on lines
+1330
to
+1359
| return () => { | ||
| // Stop any in-flight async validation or listener work tied to this instance. | ||
| for (const [key, timeout] of Object.entries( | ||
| this.timeoutIds.validations, | ||
| )) { | ||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| this.timeoutIds.validations[ | ||
| key as keyof typeof this.timeoutIds.validations | ||
| ] = null | ||
| } | ||
| } | ||
| for (const [key, timeout] of Object.entries(this.timeoutIds.listeners)) { | ||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| this.timeoutIds.listeners[ | ||
| key as keyof typeof this.timeoutIds.listeners | ||
| ] = null | ||
| } | ||
| } | ||
| for (const [key, timeout] of Object.entries( | ||
| this.timeoutIds.formListeners, | ||
| )) { | ||
| if (timeout) { | ||
| clearTimeout(timeout) | ||
| this.timeoutIds.formListeners[ | ||
| key as keyof typeof this.timeoutIds.formListeners | ||
| ] = null | ||
| } | ||
| } |
There was a problem hiding this comment.
Linked-field async work is not owned by the field being unmounted.
These loops only clear handles stored on this, but validateAsync stores linked-field work on the source field via this.getInfo().validationMetaMap[...] on Line 1871 and this.timeoutIds.validations[...] on Lines 1880-1884 even when field !== this. If a hidden field uses onChangeListenTo/onBlurListenTo with async validators, its teardown cannot cancel already-enqueued work and stale errors can still land after unmount. Please move timeout/controller ownership to field so the target field's cleanup can actually stop it.
🤖 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 1330 - 1359, Currently async
validation/listener timeouts/controllers are attached to the active instance
(this) even when the async work targets a different field, so the target field
cannot cancel them on unmount; update the code that schedules linked-field async
work (the validateAsync caller that writes to getInfo().validationMetaMap and
this.timeoutIds.validations / this.timeoutIds.listeners) to store ownership on
the target field instead of this (i.e., use the target field's timeoutIds and
validationMetaMap entry keys), and update the teardown returned by the FieldApi
cleanup to clear any timeouts/controllers stored on the field itself (iterate
and clear entries on field.timeoutIds.validations/listeners/formListeners and
remove related validationMetaMap entries) so the target field’s unmount cancels
its own async work. Ensure you reference and update validateAsync,
getInfo().validationMetaMap, and timeoutIds.validations/listeners/formListeners
usage sites so ownership is moved to the field.
Comment on lines
+1364
to
+1375
| // If a newer field instance has already been mounted for this name, | ||
| // avoid touching its shared validation state during teardown. | ||
| if (fieldInfo.instance !== this) return | ||
|
|
||
| for (const [key, validationMeta] of Object.entries( | ||
| fieldInfo.validationMetaMap, | ||
| )) { | ||
| validationMeta?.lastAbortController.abort() | ||
| fieldInfo.validationMetaMap[ | ||
| key as keyof typeof fieldInfo.validationMetaMap | ||
| ] = undefined | ||
| } |
There was a problem hiding this comment.
Older-instance async validators can still write stale meta after a remount.
The guard on Line 1366 prevents aborting the newer instance's shared controller, but it also skips aborting work started by the older instance. Those promises still commit through field.setMeta(...) on Line 1922, so an old field can overwrite the remounted field's shared meta once it resolves. Please either track abort controllers per FieldApi instance or ignore async completions when field.getInfo().instance !== field.
🤖 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 1364 - 1375, The teardown
skips aborting async work started by the older FieldApi instance because it only
avoids touching shared controllers when a newer instance is mounted; fix by
either tracking abort controllers per FieldApi instance or by ignoring async
completions when the instance no longer matches. Concretely: change the
validation logic so each FieldApi stores its own controllers (e.g., a
per-instance map on this) instead of reusing
fieldInfo.validationMetaMap.lastAbortController, and abort only those on
teardown; or add a guard in the async validator completion path (before calling
field.setMeta(...) / inside FieldApi's async result handling) that checks
field.getInfo().instance === this and returns early if it differs. Ensure you
reference validationMetaMap, lastAbortController, FieldApi, setMeta and getInfo
when locating the relevant code to update.
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.
Add unmounting to fields.
field.mountnow returns a cleanup function for unmounting.Without unmounting, conditionally rendered fields will keep their field-level validations running even though the field isn’t rendered anymore.
Summary by CodeRabbit
New Features
Bug Fixes
Tests