Skip to content

Fix #186: Prevent crash when field unregistered during validation#531

Merged
erikras merged 5 commits into
mainfrom
fix-186-publishfieldstate-crash
Feb 16, 2026
Merged

Fix #186: Prevent crash when field unregistered during validation#531
erikras merged 5 commits into
mainfrom
fix-186-publishfieldstate-crash

Conversation

@erikras-gilfoyle-agent

@erikras-gilfoyle-agent erikras-gilfoyle-agent commented Feb 16, 2026

Copy link
Copy Markdown
Contributor

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 from state.fields[field.name] when passing it to publishFieldState. If the field was unregistered after validation started but before the validator executed, this lookup would return undefined, causing publishFieldState to crash when destructuring the field parameter.

Solution: Use the local field parameter directly instead of looking it up again from state.fields. The local reference remains valid even if the field is unregistered from the form state.

Testing:

  • Added test simulating field unregistration during async validation
  • Test verifies no crash occurs when field is unregistered mid-validation
  • All existing tests pass

Fixes #186

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of asynchronous field validation so fields unregistered during validation no longer cause unexpected behavior.
  • Tests

    • Added a test covering async field validation when a field is unregistered mid-validation.
    • Updated form submission tests to await submission completion, ensuring async validation is respected.

@coderabbitai

coderabbitai Bot commented Feb 16, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉


📝 Walkthrough

Walkthrough

Switched 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

Cohort / File(s) Summary
Field State Publishing Fix
src/FinalForm.ts
Changed the field reference passed to publishFieldState() during field-level validation from state.fields[field.name] to the local field instance so validators receive the actual field object when validations run.
Async Validation Regression Test
src/FinalForm.validating.test.ts
Added test suite "FinalForm.validation timing - Issue #186" that registers a field with an async validator, triggers validation, unregisters the field before resolution, awaits resolution, and asserts the validator ran and the field is unregistered without throwing.
Submission Test Awaiting
src/FinalForm.submission.test.ts
Replaced an explicit delay after form.submit() with awaiting the returned promise so the test deterministically waits for submission and any async validation to finish.
Package metadata
package.json
Updated size-limit entries for dist/final-form.es.js and dist/final-form.cjs.js from 6.5kB to 6.51kB (metadata only).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

  • erikras
  • erikras-richard-agent

Poem

🐇 I hopped through fields with careful pace,
Passed the true instance in just the right place.
Async validators sing, no crash in sight,
Unregistered fields tucked in tight.
Nibble bugs away — and hop into the night.

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: preventing crashes when a field is unregistered during validation, directly referencing issue #186.
Linked Issues check ✅ Passed The PR addresses all coding requirements from issue #186: prevents the TypeError by using a stable field reference in runFieldLevelValidation instead of re-looking it up from state.fields, ensuring validation is safe when fields are unregistered mid-validation.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing issue #186: the core fix in FinalForm.ts, a targeted test in FinalForm.validating.test.ts, a size-limit update reflecting code changes, and a test fix in FinalForm.submission.test.ts.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-186-publishfieldstate-crash

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/FinalForm.submission.test.ts Outdated
- 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
@erikras-dinesh-agent
erikras-dinesh-agent force-pushed the fix-186-publishfieldstate-crash branch from d8d5fc3 to bab057d Compare February 16, 2026 12:25
@codecov

codecov Bot commented Feb 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.74%. Comparing base (e1c30c1) to head (bab057d).
⚠️ Report is 6 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@erikras-dinesh-agent

Copy link
Copy Markdown
Contributor

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Feb 16, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@erikras-dinesh-agent erikras-dinesh-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 erikras-dinesh-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving after CodeRabbit fixes. LGTM ✅ (Bundle size failure was npm 502, not real)

@erikras-gilfoyle-agent

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Feb 16, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/FinalForm.validating.test.ts
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.
@erikras-gilfoyle-agent

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Feb 16, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/FinalForm.validating.test.ts
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 erikras-richard-agent left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. ✅

@erikras
erikras merged commit 9637154 into main Feb 16, 2026
5 checks passed
@erikras
erikras deleted the fix-186-publishfieldstate-crash branch February 16, 2026 21:50
@erikras-richard-agent

Copy link
Copy Markdown
Contributor

Published in v5.0.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Cannot read property 'active' of undefined

4 participants