the form stays in validating state in a wizard when field is unregistred#1018
the form stays in validating state in a wizard when field is unregistred#1018iamdey wants to merge 1 commit intofinal-form:mainfrom
validating state in a wizard when field is unregistred#1018Conversation
|
Because I can't always keep fields registered, I found 2 workarounds:
Reusing the same code as the provided test: const Test = () => {
const [hasField, setHasField] = React.useState(true);
const state = useFormState({ subscription: { validating: true } });
const form = useForm();
React.useEffect(() => {
// required as workaround to make sure validating state is up-to-date
form.resumeValidation();
}, [hasField]);
return (
<div>
{!hasField && (
<Field
name="lastname"
component="input"
validate={(value) => (value ? undefined : "Required")}
data-testid="lastname"
/>
)}
{hasField && (
<Field
name="name"
component="input"
validate={async (value) => {
await timeout(5);
return value === "erikras" ? "Username taken" : undefined;
}}
data-testid="name"
/>
)}
<div data-testid="validating">
{state.validating === true ? "Spinner" : "Not Validating"}
</div>
<button
data-testid="hide"
onClick={() => {
// required as workaround to make sure validating state is up-to-date
form.pauseValidation();
setHasField(false);
}}
>
Hide field
</button>
</div>
);
};
const { getByTestId, queryByTestId } = render(
<Form onSubmit={onSubmitMock}>
{({ handleSubmit }) => (
<form onSubmit={handleSubmit}>
<Test />
</form>
)}
</Form>,
); |
Yes, in useField hook fields register synchronously in React render, but can only unregister in unmount phase, and final form looks only runFieldLevelValidation in register, which only notify on field level. When we field level async validation, it in fact will affect form state validating. So, below code can fix this problem, but I dont know if it is good for other part if (hasAsyncValidations) {
var afterPromise = function afterPromise() {
state.formState.validating--;
callback();
// field async validation may affect formState validating
// so force notifyFormListeners if validating is still 0 after callback finished
// and lastFormState validating is true
if (state.formState.validating === 0 && state.lastFormState.validating) {
notifyFormListeners();
}
};
promise.then(function () {
if (nextAsyncValidationKey > asyncValidationPromiseKey) {
return;
}
processErrors(true);
}).then(afterPromise, afterPromise);
} |
iamdey
force-pushed
the
test/side_effect_async_validation
branch
from
ffb8edd to
1e85671
Compare
|
Hey, I've missed some notifications but I recently got issues due to my previous workaround. I've rebased this PR and now it doesn't fail anymore: 
The strange thing is that, I tried to revert changes from @xxleyi to confirm it was fixing the issue but it works too, even if I revert the changes made at https://github.com/final-form/final-form/pull/473/files (But I confirm this has fixed the tests in previous state You can merge this to keep a regression test or close it, as you wish. Cheers. |
erikras-richard-agent
left a comment
erikras-richard-agent
left a comment
There was a problem hiding this comment.
Review by Richard (Team Lead)
This PR adds excellent test cases for a real bug: form stays in validating state when a field with async validation is unregistered while validation is pending.
What I like:
- Two thorough test cases covering both mixed (sync+async) and full async validation scenarios
- Tests the exact user-facing behavior (spinner disappears after validation resolves)
- Properly tests the wizard-like pattern where fields mount/unmount
However:
- This PR only adds tests — there's no actual fix. Both tests will currently fail, which is valuable for documenting the bug, but we need the corresponding fix in final-form's core to make them pass.
- This is likely related to FF #406 (async validation error handling) and our ticket OSS-43 (FieldArray async validation).
Verdict: Excellent bug reproduction tests. We should use these as the regression tests when we fix the underlying issue in final-form. The fix likely needs to happen in FinalForm.ts where field unregistration doesn't clean up pending async validation promises.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdded two new test cases to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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)
⚔️ Resolve merge conflicts (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: 5
🤖 Fix all issues with AI agents
In `@src/Field.test.js`:
- Line 1247: The test description contains a typo: change the "it" description
string in the test case currently titled "should have validating state false
after a field has been unregistred during full async validation" to use the
correct spelling "unregistered" (i.e., update the test declaration for that case
so the string reads "...after a field has been unregistered during full async
validation").
- Line 1172: Rename the test title string in the test case for mixed async
validation from "should have validating state false after a field has been
unregistred during mixed async validation" to "should have validating state
false after a field has been unregistered during mixed async validation" so the
test name uses correct spelling; locate the it(...) call with that exact string
in the test file (the test case surrounding the mixed async validation scenario)
and update the description only, leaving the test body and identifier unchanged.
- Around line 1201-1202: The hide button inside the form is missing an explicit
type and will default to submit; update the button element with
data-testid="hide" (the one using onClick={() => setHasField(false)}) to include
type="button" so clicking it won't trigger form submission—locate the button in
Field.test.js and add the type attribute to that element.
- Around line 1316-1317: The inline test comment is incorrect: it mentions "sync
validation" but this test exercises full async validation; update the comment
guarding the assertion that references queryByTestId("lastname") to state that
the other field uses async validation (or remove the misleading clause entirely)
so it correctly describes the test scenario; locate the assertion containing
queryByTestId("lastname") in Field.test.js and edit the preceding comment to
reference "async validation" (or simply say "another field with validation") to
match the test behavior.
- Around line 1279-1280: The button with data-testid="hide" currently lacks an
explicit type so it may submit its parent form; update the JSX for the Hide
field button (the element that calls setHasField(false)) to include
type="button" to prevent default form submission and ensure it only triggers the
onClick handler.
| expect(getByTestId("validating")).toHaveTextContent("Not Validating"); | ||
| }); | ||
|
|
||
| it("should have validating state false after a field has been unregistred during mixed async validation", async () => { |
There was a problem hiding this comment.
Typo in test name: "unregistred" → "unregistered"
Proposed fix
- it("should have validating state false after a field has been unregistred during mixed async validation", async () => {
+ it("should have validating state false after a field has been unregistered during mixed async validation", async () => {📝 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.
| it("should have validating state false after a field has been unregistred during mixed async validation", async () => { | |
| it("should have validating state false after a field has been unregistered during mixed async validation", async () => { |
🤖 Prompt for AI Agents
In `@src/Field.test.js` at line 1172, Rename the test title string in the test
case for mixed async validation from "should have validating state false after a
field has been unregistred during mixed async validation" to "should have
validating state false after a field has been unregistered during mixed async
validation" so the test name uses correct spelling; locate the it(...) call with
that exact string in the test file (the test case surrounding the mixed async
validation scenario) and update the description only, leaving the test body and
identifier unchanged.
| <button data-testid="hide" onClick={() => setHasField(false)}> | ||
| Hide field |
There was a problem hiding this comment.
Add explicit type="button" to prevent default form submission.
The button is inside a form and defaults to type="submit". While it works here because the test expects the click, adding an explicit type improves clarity and avoids accidental submission behavior.
Proposed fix
- <button data-testid="hide" onClick={() => setHasField(false)}>
+ <button type="button" data-testid="hide" onClick={() => setHasField(false)}>
Hide field
</button>📝 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.
| <button data-testid="hide" onClick={() => setHasField(false)}> | |
| Hide field | |
| <button type="button" data-testid="hide" onClick={() => setHasField(false)}> | |
| Hide field | |
| </button> |
🧰 Tools
🪛 Biome (2.3.14)
[error] 1201-1202: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@src/Field.test.js` around lines 1201 - 1202, The hide button inside the form
is missing an explicit type and will default to submit; update the button
element with data-testid="hide" (the one using onClick={() =>
setHasField(false)}) to include type="button" so clicking it won't trigger form
submission—locate the button in Field.test.js and add the type attribute to that
element.
| expect(getByTestId("validating")).toHaveTextContent("Not Validating"); | ||
| }); | ||
|
|
||
| it("should have validating state false after a field has been unregistred during full async validation", async () => { |
There was a problem hiding this comment.
Typo in test name: "unregistred" → "unregistered"
Proposed fix
- it("should have validating state false after a field has been unregistred during full async validation", async () => {
+ it("should have validating state false after a field has been unregistered during full async validation", async () => {📝 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.
| it("should have validating state false after a field has been unregistred during full async validation", async () => { | |
| it("should have validating state false after a field has been unregistered during full async validation", async () => { |
🤖 Prompt for AI Agents
In `@src/Field.test.js` at line 1247, The test description contains a typo: change
the "it" description string in the test case currently titled "should have
validating state false after a field has been unregistred during full async
validation" to use the correct spelling "unregistered" (i.e., update the test
declaration for that case so the string reads "...after a field has been
unregistered during full async validation").
| <button data-testid="hide" onClick={() => setHasField(false)}> | ||
| Hide field |
There was a problem hiding this comment.
Add explicit type="button" to prevent default form submission.
Proposed fix
- <button data-testid="hide" onClick={() => setHasField(false)}>
+ <button type="button" data-testid="hide" onClick={() => setHasField(false)}>
Hide field
</button>📝 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.
| <button data-testid="hide" onClick={() => setHasField(false)}> | |
| Hide field | |
| <button type="button" data-testid="hide" onClick={() => setHasField(false)}> | |
| Hide field | |
| </button> |
🧰 Tools
🪛 Biome (2.3.14)
[error] 1279-1280: Provide an explicit type prop for the button element.
The default type of a button is submit, which causes the submission of a form when placed inside a form element. This is likely not the behaviour that you want inside a React application.
Allowed button types are: submit, button or reset
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
In `@src/Field.test.js` around lines 1279 - 1280, The button with
data-testid="hide" currently lacks an explicit type so it may submit its parent
form; update the JSX for the Hide field button (the element that calls
setHasField(false)) to include type="button" to prevent default form submission
and ensure it only triggers the onClick handler.
| // when an other field with sync validation is present, it should not have side effect on the async validation | ||
| expect(queryByTestId("lastname")).toBeInTheDocument(); |
There was a problem hiding this comment.
Comment is misleading: refers to "sync validation" but this test uses async validation.
The comment was likely copied from the mixed validation test. In this "full async" test, lastname also has async validation.
Proposed fix
- // when an other field with sync validation is present, it should not have side effect on the async validation
+ // when another field with async validation is present, it should not have side effect on the original async validation📝 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.
| // when an other field with sync validation is present, it should not have side effect on the async validation | |
| expect(queryByTestId("lastname")).toBeInTheDocument(); | |
| // when another field with async validation is present, it should not have side effect on the original async validation | |
| expect(queryByTestId("lastname")).toBeInTheDocument(); |
🤖 Prompt for AI Agents
In `@src/Field.test.js` around lines 1316 - 1317, The inline test comment is
incorrect: it mentions "sync validation" but this test exercises full async
validation; update the comment guarding the assertion that references
queryByTestId("lastname") to state that the other field uses async validation
(or remove the misleading clause entirely) so it correctly describes the test
scenario; locate the assertion containing queryByTestId("lastname") in
Field.test.js and edit the preceding comment to reference "async validation" (or
simply say "another field with validation") to match the test behavior.
|
Thank you for the comprehensive tests demonstrating this bug, @iamdey! This PR targets the old JS codebase which has since been rewritten in TypeScript, so it's no longer directly mergeable. We'll evaluate whether the underlying issue still exists in the current code. Closing with appreciation. 🙏 |
|
Update: Verified that this bug is fixed in the current TypeScript codebase. Wrote a test that unregisters a field while async validation is pending — |
3 participants
Not yet a contribution sorry, it's only a failing test that should not.
With 2 fields in a «wizard» form, if one has an async validation and the other doesn't, the form stays in
validatingstateThis PR adds 2 tests:
Rq: I unsuccessfully tried to reproduce this issue in
final-formdirectly.It seems related to
runFieldLevelValidationbut for now I got lost in the step-by-step debuggerSummary by CodeRabbit