Fix #186: Prevent crash when field unregistered during validation#531
Conversation
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughSwitched which field instance is passed to field-level validators, added tests covering unregistering a field during async validation, changed one submission test to await submit, and updated package.json size metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/FinalForm.submission.test.ts`:
- Around line 1265-1301: The test uses fixed setTimeout delays which can be
flaky; replace the first await new Promise(...) after calling form.submit() with
awaiting the submit promise (await form.submit()) so the test waits
deterministically for submission to complete, and after triggering the second
validation with form.change("foo", "baz") await the existing sleep() helper (or
another microtask tick) instead of a setTimeout to let validation run; keep
references to validationCallCount and validationError assertions and ensure
console.error.mockRestore() is still called to restore the spy.
In `@src/FinalForm.validating.test.ts`:
- Around line 1746-1780: Replace the ad-hoc setTimeout-based waits and the no-op
assertion with the shared sleep() helper and a meaningful assertion: use
sleep(50) instead of new Promise(...setTimeout...) to wait for async validation,
and remove expect(true). Keep the same setup using createForm,
form.registerField, unregister() and form.change("conditional", "test"), then
after await sleep(50) assert expected state (e.g., that the field is absent or
no error thrown) to mirror other tests in the file.
- Use local field reference instead of looking up in state.fields - Prevents 'Cannot read property active of undefined' crash - Test verifies field can be unregistered while async validation runs
d8d5fc3 to
bab057d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #531 +/- ##
==========================================
- Coverage 99.05% 98.74% -0.31%
==========================================
Files 13 13
Lines 849 879 +30
Branches 286 299 +13
==========================================
+ Hits 841 868 +27
- Misses 8 11 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
erikras-dinesh-agent
left a comment
There was a problem hiding this comment.
LGTM! ✅
Clean one-line fix: using the local field reference instead of state.fields[field.name] in the validator callback avoids the crash when a field gets unregistered mid-validation. The test correctly reproduces the race condition. Bundle size bump justified.
Approved — waiting for CodeRabbit re-review to clear stale status.
- Replace setTimeout with deterministic await form.submit() - Use sleep() helper instead of setTimeout for async waits - Replace no-op assertion with meaningful state check
erikras-dinesh-agent
left a comment
There was a problem hiding this comment.
Re-approving after CodeRabbit fixes. LGTM ✅ (Bundle size failure was npm 502, not real)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/FinalForm.validating.test.ts`:
- Around line 1746-1780: The test may be a false positive because the async
validator returned by getValidator might never be invoked; modify the test to
spy or instrument the validator returned in form.registerField's getValidator
(reference getValidator and the returned async validator) and assert that the
validator was called after awaiting the sleep (or await the validator
invocation), then proceed to unregister and verify
form.getFieldState("conditional") is undefined; ensure you attach the spy before
calling form.change("conditional", "test") so the invocation is observed.
Per CodeRabbit review: add jest.fn() spy to confirm the async validator actually runs before the field is unregistered. This verifies the race condition is properly tested.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/FinalForm.validating.test.ts`:
- Around line 1752-1781: The test currently uses validatorSpy as a 1‑arg async
validator so the 3‑arg (meta-aware) validation path isn't exercised; change
validatorSpy to accept three args (value, values, meta) and use/inspect meta
inside the spy to ensure the meta-aware branch runs, keep the async sleep
behavior, and update the expectation to assert validatorSpy was called with
("test", { conditional: "test" }, expect.anything()) or specifically assert that
the third argument (meta) is defined; the changes should be applied around the
validatorSpy declaration and the expect assertion while leaving
form.registerField, unregister(), and the sleep timing intact.
Per CodeRabbit/Richard review: Changed validator from 1-arg to 3-arg (value, allValues, meta) to trigger the code path where publishFieldState(state.formState, field) is called - this is where the bug actually occurs. Updated assertions to verify meta param is present, confirming the 3-arg path was exercised.
erikras-richard-agent
left a comment
There was a problem hiding this comment.
LGTM! The fix is correct — using field directly instead of state.fields[field.name] in publishFieldState avoids the crash when a field is unregistered during async validation. The test now properly exercises the 3-arg validator path that triggers publishFieldState, and verifies meta is passed. CI all green. ✅
|
Published in |
Problem: When a field is unregistered while async validation is running, the form crashes with "Cannot read property 'active' of undefined". This happens when validators receive meta information (3-argument validators).
Root Cause: In
runFieldLevelValidation, the code looked up the field fromstate.fields[field.name]when passing it topublishFieldState. If the field was unregistered after validation started but before the validator executed, this lookup would returnundefined, causingpublishFieldStateto crash when destructuring the field parameter.Solution: Use the local
fieldparameter directly instead of looking it up again fromstate.fields. The local reference remains valid even if the field is unregistered from the form state.Testing:
Fixes #186
Summary by CodeRabbit
Bug Fixes
Tests