-
Notifications
You must be signed in to change notification settings - Fork 30
fix(components): Prevent invalid label from causing infinite loop with Autocomplete #2885
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(components): Prevent invalid label from causing infinite loop with Autocomplete #2885
Conversation
When a single value is supplied, we should do an extra check to ensure the label is not empty. This reduces the likelihood of infinite loops when consumers supply incorrect values to Autocomplete v2.
An example of an invalid label is an empty string:
```tsx
value={{label: '' }}
```
This case above is a realistic case where the consumer did not have a selection, and instead should have supplied `undefined` in this case.
Checking for a valid label is just an extra precaution we're putting in place to prevent bad loops caused by our useAutocomplete internal hook logic.
|
|
||
| describe("invalid value handling", () => { | ||
| describe("when inputValue is empty and value has empty label", () => { | ||
| it("does not call onChange", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test was added before the fix, and was failing as expected. Now passing 👍
Deploying atlantis with
|
| Latest commit: |
8e8da81
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://738c45df.atlantis.pages.dev |
| Branch Preview URL: | https://job-148590-prevent-potential.atlantis.pages.dev |
| @@ -1,3 +1,5 @@ | |||
| /* eslint-disable max-statements */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no idea why I didn't do this to begin with haha
padding my number of lines changed I suppose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure we could disable globally for all test files. That's for another day..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW the LLM went along and followed the same pattern lol
Motivations
This PR introduces an extra safety check for the Autocomplete v2
valueprop.There's a case in consumer code where they supplied an empty label (equivalent to this
value={{ label: "" }}) instead of supplyingundefinedwhen nothing is selected. This empty label can lead to an infinite loop due tovaluebeing non-null,useAutocompletewas treating that ashasSelection=truewhich then led to an effect running unnecessarily, to clear the value withonChange(undefined). The consumer code then did the exact same thing in response to thatonChangecall, supplying an empty label, thus causing the loop.This is now fixed by treating an empty/nullish
value.labelas an invalid value, making it match the unselected case just like ifvaluewasundefined. This prevents consumer code from accidentally triggering this code path leading to an infinite loop.Ideally consumers don't supply invalid labels. However, it's possibly due to the fact that ReactHookForm does not allow passing
undefinedto the controller, so consumers are working around that in their own ways. Regardless, adding this precaution is a pretty cheap and effective way to avoid potential infinite loops that end up coming back to us for investigation.Changes
Fixed
Testing
docs/components/Autocomplete/WebV2.stories.tsxTemplateFlatto the code example belowChanges can be tested via Pre-release
In Atlantis we use Github's built in pull request reviews.