Fix: Guard against undefined state in setIn() calls (fixes #909)#535
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdded defensive guards so field initialization uses an object when Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FinalForm.submission.test.ts (1)
1257-1261:⚠️ Potential issue | 🔴 CriticalAlign async-validation error test with the new rejection handling.
Line 1260 expects no submit, but rejections are caught and treated as “no errors,” so submission proceeds and CI fails. Consider awaiting submit and asserting it was called (or change the implementation if submission should be blocked).
💡 Suggested test update
- form.submit(); - await new Promise((resolve) => setTimeout(resolve, 100)); - expect(onSubmit).not.toHaveBeenCalled(); + await form.submit(); + expect(onSubmit).toHaveBeenCalledTimes(1);
🤖 Fix all issues with AI agents
In `@src/FinalForm.submission.test.ts`:
- Around line 1384-1405: The test is incorrectly passing rejectingValidator as
the 4th positional argument to form.registerField (causing duplicate
registration) instead of supplying it via the FieldConfig.getValidator property;
update the call to form.registerField that currently passes rejectingValidator
as the fourth arg to pass a single FieldConfig object containing getValidator:
rejectingValidator (and ensure you only register the field once by using the
existing field mock and change from field.mock.calls[0][0] rather than
re-calling registerField twice).
In `@src/FinalForm.ts`:
- Around line 353-359: The .catch() handlers that currently swallow rejected
async validation promises should log the rejection before clearing errors:
update the catch blocks attached to the promise returned by errorsOrPromise (and
the other similar catch at the 421-433 range) to accept an error parameter and
call console.error(error) (or processLogger/error helper if that module uses
one) immediately prior to calling setErrors({}, true) so rejections are visible
while still treating them as no errors.
- Around line 233-238: The forEach arrow currently uses an implicit return which
Biome flags; update the forEach callback in the scheduler block that processes
pendingAsyncCallbacks so it uses a block body and calls the callback inside it
(i.e., change the lambda passed to callbacks.forEach to use { callback(); }
instead of the expression form), leaving the rest of the logic
(pendingAsyncCallbacks clearing and asyncCallbacksScheduled reset) unchanged.
| scheduler(() => { | ||
| const callbacks = [...pendingAsyncCallbacks]; | ||
| pendingAsyncCallbacks.length = 0; | ||
| asyncCallbacksScheduled = false; | ||
| callbacks.forEach(callback => callback()); | ||
| callbacks.forEach((callback) => callback()); | ||
| }); |
There was a problem hiding this comment.
Fix Biome lint: avoid returning a value from forEach callback.
Biome flags the implicit return from callbacks.forEach((callback) => callback()). Use a block body instead.
✅ Lint-safe change
- callbacks.forEach((callback) => callback());
+ callbacks.forEach((callback) => {
+ callback();
+ });📝 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.
| scheduler(() => { | |
| const callbacks = [...pendingAsyncCallbacks]; | |
| pendingAsyncCallbacks.length = 0; | |
| asyncCallbacksScheduled = false; | |
| callbacks.forEach(callback => callback()); | |
| callbacks.forEach((callback) => callback()); | |
| }); | |
| scheduler(() => { | |
| const callbacks = [...pendingAsyncCallbacks]; | |
| pendingAsyncCallbacks.length = 0; | |
| asyncCallbacksScheduled = false; | |
| callbacks.forEach((callback) => { | |
| callback(); | |
| }); | |
| }); |
🧰 Tools
🪛 Biome (2.3.14)
[error] 237-237: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/FinalForm.ts` around lines 233 - 238, The forEach arrow currently uses an
implicit return which Biome flags; update the forEach callback in the scheduler
block that processes pendingAsyncCallbacks so it uses a block body and calls the
callback inside it (i.e., change the lambda passed to callbacks.forEach to use {
callback(); } instead of the expression form), leaving the rest of the logic
(pendingAsyncCallbacks clearing and asyncCallbacksScheduled reset) unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test-rejected-validation.js`:
- Around line 1-47: This manual script file contains only console logs, imports
the built bundle via createForm from final-form.cjs.js, and leaves an unused
submitPromise; either delete this file or convert it into a proper Jest test
that imports the source module (use the library entrypoint used by other tests),
replaces console logs with assertions around createForm, form.change,
form.submit (or await the returned submitPromise) and form.getState, and remove
the unused submitPromise by awaiting form.submit() (or asserting on its
resolved/rejected value); locate symbols test(), createForm, form.change,
form.submit and form.getState in the file to implement the change.
Review from Richard 🤓Several issues need fixing before I can approve: 🚨 Critical
|
- Add defensive checks when calling setIn() with formState.values - Ensures empty object fallback when values is undefined - Prevents 'Cannot call setIn() with undefined state' error - Occurs with keepDirtyOnReinitialize when all values are removed - Add tests for initialValue and defaultValue scenarios - Bump bundle size limits to 6.51kB
69dee03 to
bd6f276
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #535 +/- ##
==========================================
- Coverage 99.05% 98.52% -0.53%
==========================================
Files 13 13
Lines 849 883 +34
Branches 286 305 +19
==========================================
+ Hits 841 870 +29
- Misses 8 13 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
erikras-dinesh-agent
left a comment
erikras-dinesh-agent
left a comment
There was a problem hiding this comment.
LGTM! ✅
Clean fix for #909. Guards state.formState.values with || {} before passing to setIn(), preventing the crash when keepDirtyOnReinitialize + initialize({}) leaves values undefined. Two thorough tests covering both initialValue and defaultValue registration paths. Bundle size bump justified.
Approved.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/FinalForm.submission.test.ts`:
- Around line 1379-1380: The two test cases starting with it("should not crash
when registering field with initialValue after values becomes undefined (issue
`#909`)"...) are placed outside the describe("FinalForm.submission", ...) block;
move both of these it(...) tests into that describe block (so they are nested
under FinalForm.submission) and ensure the describe block's closing brace is
moved to after the second test so the grouping and filtering remain correct.
Per CodeRabbit review: Tests should be inside the FinalForm.submission describe block. Created new nested describe for Issue #909.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
LGTM! The || {} fallback for state.formState.values is the right fix — prevents the crash when keepDirtyOnReinitialize + initialize({}) leaves values undefined. Tests are well-structured and cover both initialValue and defaultValue paths. CI all green. ✅
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/FinalForm.submission.test.ts`:
- Around line 1391-1444: The tests currently leave field1 dirty so
keepDirtyOnReinitialize preserves its key and the regression isn't exercised;
before calling form.initialize({}) call the field's change back to its original
value (e.g., invoke the change function from
field1.mock.calls[0][0].change("value1") or otherwise clear the dirty flag) so
that initialize({}) will produce undefined formState.values, then assert
form.getState().values is undefined to confirm the state that previously
crashed; apply this same change to both test cases surrounding registerField and
use references: createForm(...), form.registerField(...),
field1.mock.calls[0][0].change(...), form.initialize({}), and
form.getState().values.
| // Change the value to make it dirty | ||
| const { change } = field1.mock.calls[0][0]; | ||
| change("modified"); | ||
|
|
||
| // Reinitialize with empty values - this can make formState.values undefined | ||
| // when keepDirtyOnReinitialize removes all keys | ||
| form.initialize({}); | ||
|
|
||
| // This should not crash with "Cannot call setIn() with undefined state" | ||
| const field2 = jest.fn(); | ||
| expect(() => { | ||
| form.registerField( | ||
| "field2", | ||
| field2, | ||
| { value: true }, | ||
| { | ||
| initialValue: "newValue", | ||
| }, | ||
| ); | ||
| }).not.toThrow(); | ||
|
|
||
| // Verify the field was registered successfully | ||
| expect(field2).toHaveBeenCalled(); | ||
| expect(field2.mock.calls[0][0].value).toBe("newValue"); | ||
| }); | ||
|
|
||
| it("should not crash when registering field with defaultValue after values becomes undefined", () => { | ||
| const onSubmit = jest.fn(); | ||
| const form = createForm({ | ||
| onSubmit, | ||
| keepDirtyOnReinitialize: true, | ||
| initialValues: { field1: "value1" }, | ||
| }); | ||
|
|
||
| const field1 = jest.fn(); | ||
| form.registerField("field1", field1, { value: true }); | ||
|
|
||
| // Change to make dirty, then reinitialize with empty | ||
| const { change } = field1.mock.calls[0][0]; | ||
| change("modified"); | ||
| form.initialize({}); | ||
|
|
||
| // This should not crash with "Cannot call setIn() with undefined state" | ||
| const field2 = jest.fn(); | ||
| expect(() => { | ||
| form.registerField( | ||
| "field2", | ||
| field2, | ||
| { value: true }, | ||
| { | ||
| defaultValue: "defaultValue", | ||
| }, | ||
| ); | ||
| }).not.toThrow(); |
There was a problem hiding this comment.
Ensure the regression tests actually reach the undefined-values state.
Line 1391-1397 makes field1 dirty, which keepDirtyOnReinitialize usually preserves, so formState.values may remain defined and the test could pass even without the fix. Consider clearing dirty values before initialize({}) and asserting form.getState().values is undefined (or otherwise force the state that previously crashed) in both tests to make the regression meaningful.
🧪 Suggested hardening (apply similarly in both tests)
- change("modified");
+ change("modified");
+ change("value1"); // clear dirty so values can be dropped on reinit
form.initialize({});
+ expect(form.getState().values).toBeUndefined();📝 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.
| // Change the value to make it dirty | |
| const { change } = field1.mock.calls[0][0]; | |
| change("modified"); | |
| // Reinitialize with empty values - this can make formState.values undefined | |
| // when keepDirtyOnReinitialize removes all keys | |
| form.initialize({}); | |
| // This should not crash with "Cannot call setIn() with undefined state" | |
| const field2 = jest.fn(); | |
| expect(() => { | |
| form.registerField( | |
| "field2", | |
| field2, | |
| { value: true }, | |
| { | |
| initialValue: "newValue", | |
| }, | |
| ); | |
| }).not.toThrow(); | |
| // Verify the field was registered successfully | |
| expect(field2).toHaveBeenCalled(); | |
| expect(field2.mock.calls[0][0].value).toBe("newValue"); | |
| }); | |
| it("should not crash when registering field with defaultValue after values becomes undefined", () => { | |
| const onSubmit = jest.fn(); | |
| const form = createForm({ | |
| onSubmit, | |
| keepDirtyOnReinitialize: true, | |
| initialValues: { field1: "value1" }, | |
| }); | |
| const field1 = jest.fn(); | |
| form.registerField("field1", field1, { value: true }); | |
| // Change to make dirty, then reinitialize with empty | |
| const { change } = field1.mock.calls[0][0]; | |
| change("modified"); | |
| form.initialize({}); | |
| // This should not crash with "Cannot call setIn() with undefined state" | |
| const field2 = jest.fn(); | |
| expect(() => { | |
| form.registerField( | |
| "field2", | |
| field2, | |
| { value: true }, | |
| { | |
| defaultValue: "defaultValue", | |
| }, | |
| ); | |
| }).not.toThrow(); | |
| // Change the value to make it dirty | |
| const { change } = field1.mock.calls[0][0]; | |
| change("modified"); | |
| change("value1"); // clear dirty so values can be dropped on reinit | |
| // Reinitialize with empty values - this can make formState.values undefined | |
| // when keepDirtyOnReinitialize removes all keys | |
| form.initialize({}); | |
| expect(form.getState().values).toBeUndefined(); | |
| // This should not crash with "Cannot call setIn() with undefined state" | |
| const field2 = jest.fn(); | |
| expect(() => { | |
| form.registerField( | |
| "field2", | |
| field2, | |
| { value: true }, | |
| { | |
| initialValue: "newValue", | |
| }, | |
| ); | |
| }).not.toThrow(); | |
| // Verify the field was registered successfully | |
| expect(field2).toHaveBeenCalled(); | |
| expect(field2.mock.calls[0][0].value).toBe("newValue"); | |
| }); | |
| it("should not crash when registering field with defaultValue after values becomes undefined", () => { | |
| const onSubmit = jest.fn(); | |
| const form = createForm({ | |
| onSubmit, | |
| keepDirtyOnReinitialize: true, | |
| initialValues: { field1: "value1" }, | |
| }); | |
| const field1 = jest.fn(); | |
| form.registerField("field1", field1, { value: true }); | |
| // Change to make dirty, then reinitialize with empty | |
| const { change } = field1.mock.calls[0][0]; | |
| change("modified"); | |
| change("value1"); // clear dirty so values can be dropped on reinit | |
| form.initialize({}); | |
| expect(form.getState().values).toBeUndefined(); | |
| // This should not crash with "Cannot call setIn() with undefined state" | |
| const field2 = jest.fn(); | |
| expect(() => { | |
| form.registerField( | |
| "field2", | |
| field2, | |
| { value: true }, | |
| { | |
| defaultValue: "defaultValue", | |
| }, | |
| ); | |
| }).not.toThrow(); |
🤖 Prompt for AI Agents
In `@src/FinalForm.submission.test.ts` around lines 1391 - 1444, The tests
currently leave field1 dirty so keepDirtyOnReinitialize preserves its key and
the regression isn't exercised; before calling form.initialize({}) call the
field's change back to its original value (e.g., invoke the change function from
field1.mock.calls[0][0].change("value1") or otherwise clear the dirty flag) so
that initialize({}) will produce undefined formState.values, then assert
form.getState().values is undefined to confirm the state that previously
crashed; apply this same change to both test cases surrounding registerField and
use references: createForm(...), form.registerField(...),
field1.mock.calls[0][0].change(...), form.initialize({}), and
form.getState().values.
Per CodeRabbit review: Tests weren't actually exercising the crash condition
because field1 stayed dirty, so keepDirtyOnReinitialize preserved it.
Fixed by:
1. Change field1 to 'modified' (make dirty)
2. Change field1 back to 'value1' (no longer dirty)
3. initialize({}) now creates the undefined values condition
4. Removed incorrect assertion - with the fix, values is {} not undefined
The tests now properly verify the fix prevents the crash in the scenario
that would previously cause 'Cannot call setIn() with undefined state'.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Published in |
4 participants
Problem
When using
keepDirtyOnReinitialize, the form crashes with "Cannot call setIn() with undefined state" error. This occurs when:keepDirtyOnReinitialize: trueinitialValueordefaultValueis registeredOriginal issue: react-final-form#909 (root cause in final-form)
CodeSandbox reproduction: https://codesandbox.io/s/upbeat-shaw-5ffvl
Root Cause
In
registerField(), when settinginitialValueordefaultValue, the code called:If
formState.valueswasundefined(which can happen withkeepDirtyOnReinitializeafter removing all keys),setIn()throws an error because it has a guard against undefined state.Solution
Added defensive checks with fallbacks:
This pattern:
initialize()functionTesting
Added two test cases:
initialValueafter values becomes undefined - verifies no crash and value is set correctlydefaultValueafter values becomes undefined - verifies no crash and value is set correctlyBoth tests pass with the fix.
Summary by CodeRabbit
Bug Fixes
Tests