fix(autocomplete): validate prop not working after hovering #4452
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 0abd1a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughA patch has been introduced for the Changes
Possibly related PRs
Suggested reviewers
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/autocomplete/src/use-autocomplete.ts (1)
447-447: Clarify whyisInvalidis conditionally set toundefined.A quick inline comment explaining that setting
isInvalidtoundefineddefers to the parent or custom validation logic would help future maintainers understand this workaround. This also ensures that the defaultisInvalidis not inadvertently persisted in scenarios with uncommitted validation.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/ninety-lobsters-deliver.md(1 hunks)packages/components/autocomplete/__tests__/autocomplete.test.tsx(1 hunks)packages/components/autocomplete/src/use-autocomplete.ts(1 hunks)
🔇 Additional comments (3)
packages/components/autocomplete/src/use-autocomplete.ts (1)
436-441: Validate the existence of displayValidation and realtimeValidation objects.
Although this logic effectively prevents the use-input hook from overwriting the validation state, consider adding a safety check to ensure state.displayValidation and state.realtimeValidation are always defined. If they are ever undefined, this could throw an error.
packages/components/autocomplete/__tests__/autocomplete.test.tsx (1)
742-795: Comprehensive test scenario for hovering-related validation behavior.
This new test method thoroughly validates the fix by simulating user actions that caused the bug. It effectively ensures that form submission is blocked when hovering disrupts the validation state, and that the validation message is only dismissed upon a correct selection. Great job covering a realistic user flow!
.changeset/ninety-lobsters-deliver.md (1)
1-5: Clear and concise change log.
The patch statement and description of the fix are clear, making it straightforward for users to understand the nature of this release.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/components/autocomplete/src/use-autocomplete.ts (1)
436-441: LGTM! Consider enhancing the comment for better clarity.The logic for detecting uncommitted validation errors is well-implemented. However, the comment could be more descriptive.
Consider updating the comment to better explain the validation state conflict:
- // prevent use-input's useFormValidation hook from overwriting use-autocomplete's useFormValidation hook when there are uncommitted validation errors - // see https://github.com/nextui-org/nextui/pull/4452 + // Prevents validation state conflicts between useFormValidation hooks + // When there are uncommitted validation errors: + // - use-input's useFormValidation hook attempts to overwrite the state + // - use-autocomplete's useFormValidation hook needs to maintain control + // See: https://github.com/nextui-org/nextui/pull/4452
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/autocomplete/src/use-autocomplete.ts(1 hunks)
🔇 Additional comments (1)
packages/components/autocomplete/src/use-autocomplete.ts (1)
448-448: LGTM! Let's verify the validation behavior.
The conditional setting of isInvalid to undefined effectively prevents validation state conflicts. This change aligns perfectly with the PR's objective.
Let's verify that this change doesn't affect other validation scenarios:
✅ Verification successful
Let me try a different approach to verify the validation behavior.
Let me check the test file to understand the validation scenarios.
Based on the test file and implementation details, I can now provide a conclusive response about the validation behavior change.
✓ The validation behavior change is correct and well-tested
The modification to isInvalid prop is a deliberate fix that prevents validation state conflicts between use-input and use-autocomplete hooks. The test file includes a specific test case "should work with validate after hovering" that verifies this exact scenario:
- The test confirms that hovering over the combobox no longer causes validation from
use-inputto overwrite the validation fromuse-autocomplete - It verifies that form submission correctly handles validation errors after hovering
- The implementation ensures that validation state remains consistent when there are uncommitted validation errors
Key test evidence:
it("should work with validate after hovering", async () => {
// ... test setup ...
await user.hover(combobox);
await user.click(submit);
expect(onSubmit).toHaveBeenCalledTimes(0);
expect(combobox).toHaveAttribute("aria-describedby");
// ... validation continues correctly after hover ...
});🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other validation-related code that might be affected
# Look for validation behavior patterns in tests and related components
# Search for validation-related test cases
echo "Checking validation-related test cases:"
rg -l "validation|isInvalid|validate" --type ts --type tsx packages/components/autocomplete/__tests__/
# Search for other components using useFormValidation
echo "Checking other components using useFormValidation:"
rg -l "useFormValidation" --type ts --type tsx packages/components/
# Look for similar validation patterns
echo "Checking for similar validation patterns:"
ast-grep --pattern 'isInvalid = $value ? undefined : $expr'
Length of output: 583
Script:
#!/bin/bash
# Search for validation-related test cases with correct file type
echo "Checking validation-related test cases:"
rg -l "validation|isInvalid|validate" packages/components/autocomplete/__tests__/
# Search for other components using useFormValidation
echo "Checking other components using useFormValidation:"
rg -l "useFormValidation" packages/components/
# Look for isInvalid usage in autocomplete tests
echo "Checking isInvalid usage in tests:"
rg "isInvalid" -C 3 packages/components/autocomplete/__tests__/
# Check the implementation of useFormValidation
echo "Checking useFormValidation implementation:"
fd "use-form-validation.ts" packages/components/ --exec cat {}
Length of output: 855
Script:
#!/bin/bash
# Check the autocomplete test file content
echo "Checking autocomplete test content:"
cat packages/components/autocomplete/__tests__/autocomplete.test.tsx
# Look for validation-related hooks in the component
echo "Checking validation-related hooks:"
rg "useFormValidation|useValidation" -C 5 packages/components/autocomplete/src/use-autocomplete.ts
# Check how isInvalid is used in other components
echo "Checking isInvalid usage in select and input-otp:"
rg "isInvalid" -C 3 packages/components/select/src/hidden-select.tsx
rg "isInvalid" -C 3 packages/components/input-otp/src/use-input-otp.ts
Length of output: 33351
Closes eng-1790
📝 Description
<input>that has 2useFormValidationhooks attached to it due to autocomplete's implementation using NextUI's own inputuseFormValidationhooks are attached by autocomplete'suseComboBoxand input'suseTextFielduseFormValidationis controlled externally by autocomplete throughisInvalidanderrorMessagevalidateprop's function for validation andvalidationBehavior=native, after initial render, autocomplete will be invalid but the errors do not show yet since nothing has been committeduseFormValidationwill execute due to theuseHoverhook, which will cause it to overwrite the validation result of theuseFormValidationhook fromuseComboboxuseFormValidationhooks reference the same<input>isInvalid=falseand no validation errors yetuseFormValidationwill run and set the<input>'s validity totruesince its errors externally controlled but there are no errors yetuseFormValidationhas a chance to update the<input>to the correct validation stateisInvalidtoundefinedwhen there is uncommitted validation so input'suseFormValidationdoes not overwrite that of autocompletesandbox: https://codesandbox.io/p/github/Peterl561/autocomplete-validate/main
validationBehavior=nativeform with avalidatefunctionvalidateon autocomplete does not prevent submissiona. clicking submit without hovering autocomplete works
b. clicking autocomplete (instead of only hovering) before submitting works
⛳️ Current behavior (updates)
when autocomplete first renders, repeatedly hovering the input prevent
validatefrom working as expectedbefore.mp4
🚀 New behavior
hovering autocomplete before submit does not interfere with
validateafter.mp4
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
Tests