Fix #984: useField returns stale values when sibling updates form in useEffect#1085
Fix #984: useField returns stale values when sibling updates form in useEffect#1085
Conversation
…useEffect Problem: When a parent/sibling component's useEffect changes a form value, other useField hooks see stale values because their subscription hasn't registered yet. The initial state had no-op blur/change/focus handlers. Fix: Replace no-op handlers with live form-backed handlers that call form.blur/form.change/form.focus directly, so effect-time changes propagate immediately before the permanent subscription is registered. Also includes #988 fix for radio button dirty state when initialValue changes.
|
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:
📝 WalkthroughWalkthroughUpdates ChangesuseField behavior, test, and lint
Sequence Diagram(s)sequenceDiagram
participant Parent as ParentWithEffect
participant Field1 as Field1 (useField)
participant FormAPI as FinalForm (form)
participant Field2 as Field2 (subscriber)
Parent->>Field1: mount -> useField(name="field1")
Field1->>FormAPI: registerField(name="field1")
Parent-->>FormAPI: useEffect triggers -> form.change("field1","UpdatedByField1")
FormAPI-->>Field2: notify subscribers (value updated)
Field2->>Field2: render updated value "UpdatedByField1"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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 |
![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 `@src/useField.issue-984.test.js`:
- Line 10: Remove the manual cleanup call: delete the afterEach(cleanup)
invocation (and any unused import of cleanup) from the test file since React
Testing Library (v9+) auto-cleans between tests; locate the afterEach(cleanup)
line in the test (and the cleanup import if present) and remove them to simplify
the test setup.
- Around line 51-57: The test is using a fragile async IIFE with setTimeout to
wait for state updates; replace that with React Testing Library's waitFor:
import waitFor from '@testing-library/react' (or use the exported waitFor) and
change the block that awaits the 100ms timeout to await waitFor(() =>
expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1")); remove
the setTimeout and the IIFE so the test relies on waitFor to poll for the DOM
update triggered by the useEffect.
In `@src/useField.ts`:
- Around line 237-249: The registerField call is creating an unused
subscription; capture its returned unsubscribe function from
form.registerField(name as keyof FormValues, () => {}, {}, { initialValue }) and
call that unsubscribe immediately after registering (while still inside the try
block) to avoid leaving an orphan subscriber; keep the surrounding
form.pauseValidation() and form.resumeValidation() usage and ensure the
unsubscribe is invoked before resumeValidation() is reached.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5aced691-0dfa-48f1-ad9a-0f5b563b4cb7
📒 Files selected for processing (2)
src/useField.issue-984.test.jssrc/useField.ts
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 `@src/useField.issue-984.test.js`:
- Around line 27-30: The React.useEffect in the test intentionally uses an empty
dependency array to run only on mount while calling input.onChange; to prevent
eslint from flagging this, add an ESLint disable comment above the effect (e.g.,
// eslint-disable-next-line react-hooks/exhaustive-deps) to document the intent
that input.onChange is stable (via useConstantCallback) and that the empty deps
are deliberate; place the comment immediately before the React.useEffect block
that calls input.onChange.
- Around line 51-53: The test waits for an element queried by
getByTestId("field1-value") and asserts its text via .textContent; replace that
assertion with the jest-dom matcher toHaveTextContent for clearer, idiomatic
failure messages—inside the same waitFor callback, call
expect(getByTestId("field1-value")) and assert
.toHaveTextContent("UpdatedByField1") instead, keeping the existing waitFor and
getByTestId usage.
In `@src/useField.ts`:
- Around line 224-227: The equality checks in useField that compare
prevInitialValueRef.current, initialValue, and currentValue are using strict
reference equality (!== / ===) which breaks for recreated objects/arrays; update
both comparisons to use the configured equality function: call
configRef.current.isEqual if present, otherwise fallback to (a, b) => a === b,
and use that helper when comparing prevInitialValueRef.current vs initialValue
and currentValue vs initialValue so custom equality logic is respected and
object/array initialValue props behave correctly.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b726f19e-2091-4ebc-b954-7b8e4ad0ca1b
📒 Files selected for processing (2)
src/useField.issue-984.test.jssrc/useField.ts
CodeRabbit Review Comments AddressedThe following issues flagged by CodeRabbit have been fixed in commit d7ffb05:
|
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 `@src/useField.ts`:
- Around line 243-248: The temporary call to form.registerField inside useField
re-registration passes only { initialValue } and omits the field's equality
comparator; update that call to include the configured isEqual (i.e., forward
configRef.current.isEqual) so the temporary registration uses the same equality
logic as the normal path (see where the normal registration at line ~91 forwards
configRef.current.isEqual). Ensure the options object passed to registerField
contains both initialValue and isEqual so Final Form computes dirty/pristine
using the correct comparator.
- Around line 239-252: Before calling form.pauseValidation() capture whether
validation was already paused (e.g., const wasPaused = typeof
form.isValidationPaused === 'function' ? form.isValidationPaused() : false),
then call form.pauseValidation(), perform the registerField/unsubscribe block,
and only call form.resumeValidation() if wasPaused is false (i.e., if this
effect actually paused validation). Reference the existing calls to
form.pauseValidation(), form.resumeValidation(), and form.registerField to
locate and update the logic; use optional chaining/feature-detection for
form.isValidationPaused to avoid runtime errors.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af0eb8d1-2ccb-4dd8-961f-3a98f362da28
📒 Files selected for processing (2)
src/useField.issue-984.test.jssrc/useField.ts
…ration - Check form.isValidationPaused() before calling pauseValidation() so we don't inadvertently resume validation that was already paused externally (e.g. by ReactFinalForm during setup). Mirrors the pattern used in ReactFinalForm.tsx. - Pass configRef.current.isEqual through the temporary registerField call so dirty/pristine calculation uses the field's configured equality comparator rather than the default reference equality. Addresses remaining CodeRabbit review comments.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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 `@src/useField.ts`:
- Around line 197-205: The effect uses configRef.current.isEqual to detect
initialValue changes but only assigns prevInitialValueRef.current when
initialValue !== undefined, causing missed transitions (value -> undefined ->
same value); update the ref any time the value is considered changed by isEqual
so prevInitialValueRef.current = initialValue runs regardless of undefined,
while keeping the existing change-detection branch (the useEffect in useField
that references prevInitialValueRef, initialValue and configRef.current.isEqual)
so registration/dirty state logic still runs when appropriate.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 853a6b00-0798-4de2-86ab-b93828935550
📒 Files selected for processing (2)
eslint.config.mjssrc/useField.ts
Previously the ref was only updated inside the 'initialValue !== undefined' branch, so a transition like "foo" → undefined → "foo" would leave the ref stuck at "foo" and the second change would look like a no-op, leaving dirty/pristine state stale. Move the ref update unconditionally before the condition check, while keeping the registration logic gated on 'initialValue !== undefined'. Addresses CodeRabbit review comment.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1085 +/- ##
==========================================
- Coverage 98.60% 93.22% -5.39%
==========================================
Files 18 18
Lines 359 413 +54
Branches 105 132 +27
==========================================
+ Hits 354 385 +31
- Misses 5 28 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add 5 tests covering the initialValue-change effect introduced in this PR: - Field becomes pristine when initialValue changes to match current value - Field value/initialValue update when initialValue prop changes - initialValue transitioning through undefined (value→undefined→value) - Custom isEqual used when comparing initialValue changes - Field stays pristine when unmodified and initialValue changes These tests exercise the prevInitialValueRef tracking, the isEqual-based comparison, and the pauseValidation/resumeValidation guard paths. Improves useField.ts branch coverage from ~68% to ~71%.
erikras-gilfoyle-agent
dismissed stale reviews from erikras and coderabbitai[bot]
via
a42479a
2 participants
Problem
When a parent/sibling component's
useEffectchanges a form value, otheruseFieldhooks see stale values because their subscription hasn't registered yet.The initial state had no-op
blur/change/focushandlers, so calls toinput.onChange()during the effect phase were silently dropped.Solution
Replace no-op handlers with live form-backed handlers that call
form.blur/form.change/form.focusdirectly. This ensures effect-time changes propagate immediately, before the permanent subscription is registered.Test
Added regression test in
useField.issue-984.test.jsthat verifies siblinguseFieldhooks receive updated values when another field changes the form inuseEffect.Fixes #984
Summary by CodeRabbit
Bug Fixes
Tests
Chores