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#1085erikras-richard-agent wants to merge 2 commits intomainfrom
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.
📝 WalkthroughWalkthroughAdded a Jest test reproducing a useEffect-driven value update and modified Changes
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 -> calls 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 docstrings
🧪 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: 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
| React.useEffect(() => { | ||
| // Simulate programmatic change during effect phase | ||
| input.onChange("UpdatedByField1"); | ||
| }, []); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding ESLint disable comment for exhaustive-deps.
The empty dependency array is intentional to simulate a mount-only effect. Since input.onChange is stable (via useConstantCallback), this is safe, but ESLint's react-hooks/exhaustive-deps rule would flag it. Adding a disable comment documents the intent.
♻️ Proposed fix
React.useEffect(() => {
// Simulate programmatic change during effect phase
input.onChange("UpdatedByField1");
+ // eslint-disable-next-line react-hooks/exhaustive-deps
}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| React.useEffect(() => { | |
| // Simulate programmatic change during effect phase | |
| input.onChange("UpdatedByField1"); | |
| }, []); | |
| React.useEffect(() => { | |
| // Simulate programmatic change during effect phase | |
| input.onChange("UpdatedByField1"); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/useField.issue-984.test.js` around lines 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.
| await waitFor(() => { | ||
| expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1"); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer toHaveTextContent matcher for clearer assertions.
Since @testing-library/jest-dom is imported, using toHaveTextContent provides better error messages on failure and is the idiomatic approach.
♻️ Proposed fix
await waitFor(() => {
- expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1");
+ expect(getByTestId("field1-value")).toHaveTextContent("UpdatedByField1");
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await waitFor(() => { | |
| expect(getByTestId("field1-value").textContent).toBe("UpdatedByField1"); | |
| }); | |
| await waitFor(() => { | |
| expect(getByTestId("field1-value")).toHaveTextContent("UpdatedByField1"); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/useField.issue-984.test.js` around lines 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.
| // Force an update through the field subscriber by triggering a change | ||
| // to the same value, which will recalculate dirty state with new initial | ||
| if (currentValue === initialValue) { | ||
| // The value matches the new initial, so field should become pristine. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if isEqual is used elsewhere in the codebase for value comparisons
rg -n "isEqual" --type=ts src/Repository: final-form/react-final-form
Length of output: 413
🏁 Script executed:
# Read the useField.ts file focusing on the effect at lines 193-256
sed -n '193,256p' src/useField.ts | cat -nRepository: final-form/react-final-form
Length of output: 3622
🏁 Script executed:
# Also check the config/types structure to understand isEqual availability
sed -n '1,100p' src/useField.ts | cat -nRepository: final-form/react-final-form
Length of output: 3824
🏁 Script executed:
# Check how isEqual is actually used in useWhenValueChanges for comparison patterns
sed -n '1,30p' src/useWhenValueChanges.ts | cat -nRepository: final-form/react-final-form
Length of output: 525
🏁 Script executed:
# Search for test cases using initialValue with objects/arrays or non-primitive values
rg -n "initialValue" --type=ts --type=tsx -B2 -A2 src/__tests__/ | head -100Repository: final-form/react-final-form
Length of output: 98
🏁 Script executed:
# Check if there are examples or documentation about object/array initialValues
rg -n "initialValue.*\{|\[" --type=ts --type=tsx src/ | head -50Repository: final-form/react-final-form
Length of output: 98
🏁 Script executed:
# Check what the default isEqual should be when config.isEqual is undefined
rg -n "isEqual.*=" --type=ts -B1 -A1 src/types.tsRepository: final-form/react-final-form
Length of output: 167
🏁 Script executed:
# Search for how registerField is called to see how isEqual is handled
rg -n "registerField" src/useField.ts -B2 -A10 | head -60Repository: final-form/react-final-form
Length of output: 1943
🏁 Script executed:
# Look for actual usage of initialValue with objects/arrays in tests
find src -name "*.test.ts" -o -name "*.test.js" | xargs rg -l "initialValue" 2>/dev/null | head -5Repository: final-form/react-final-form
Length of output: 207
🏁 Script executed:
# Check final-form package to see how it handles isEqual (look at node_modules or imports)
rg -n "isEqual" src/useField.ts -B3 -A3Repository: final-form/react-final-form
Length of output: 271
🏁 Script executed:
# Look at the test file for useField to see if there are object/array initialValue tests
head -200 src/useField.test.js | cat -nRepository: final-form/react-final-form
Length of output: 8734
🏁 Script executed:
# Search specifically for initialValue tests with objects
rg -n "initialValue.*\{" src/useField.test.js -A5 | head -80Repository: final-form/react-final-form
Length of output: 2529
🏁 Script executed:
# Check the issue-984 test since the comment mentions FIX `#988`
cat src/useField.issue-984.test.js | head -100Repository: final-form/react-final-form
Length of output: 1837
The strict equality comparison may fail for object/array initialValue props.
Both the condition at line 8 (prevInitialValueRef.current !== initialValue) and line 34 (currentValue === initialValue) use strict reference equality. If initialValue is an object or array recreated on each render, these comparisons could behave unexpectedly—either always true or always false depending on whether references match.
Use configRef.current.isEqual (or a fallback like (a, b) => a === b when undefined) for consistency with how Final Form handles equality elsewhere in the codebase, and to respect custom equality functions provided by users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/useField.ts` around lines 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.
1 participant
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