Skip to content
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

Bugfix/add missing bounds check for findIndex #3565

Conversation

mhwaage
Copy link
Contributor

@mhwaage mhwaage commented Jul 29, 2024

  • Adds a bounds check for Autocomplete's findIndex, to prevent infinite recursion once the index has gone beyond bounds.
  • Adds test to prevent regression

fixes an issue where a optionsDisabled function that would return true for an undefined value would crash from infinite recursion
@mhwaage
Copy link
Contributor Author

mhwaage commented Jul 29, 2024

fixes #3566

@@ -331,6 +331,28 @@ describe('Autocomplete', () => {
expect(input).toHaveValue('')
})

it('Correctly handles keypresses up/down when all options are disabled', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add it here, because of scope creep, but i suppose that a test case with a large amount (>10000) of contiguous disabled options would still fail in the same manner.

@oddvernes
Copy link
Collaborator

I thought this had been fixed by #3534 but I guess I missed something? I'll take a look when i return from vacation next week!

@mhwaage
Copy link
Contributor Author

mhwaage commented Jul 30, 2024

I thought this had been fixed by #3534 but I guess I missed something? I'll take a look when i return from vacation next week!

Possible that I implemented this fix on an outdated version, I will verify!

@mhwaage
Copy link
Contributor Author

mhwaage commented Jul 30, 2024

I thought this had been fixed by #3534 but I guess I missed something? I'll take a look when i return from vacation next week!

Possible that I implemented this fix on an outdated version, I will verify!

#3534 fixes some of the issues but still misses edge cases. I will update the tests to cover those edge cases.

@oddvernes
Copy link
Collaborator

oddvernes commented Aug 5, 2024

I was trying to reproduce the edge case but think i found another one instead.

  1. Run storybook and go to the disabled option story
  2. type "am" so only the disabled amazon option is in the list
  3. press down arrow
    Another crash in findindex

@mhwaage
Copy link
Contributor Author

mhwaage commented Aug 5, 2024

I was trying to reproduce the edge case but think i found another one instead.

  1. Run storybook and go to the disabled option story
  2. type "am" so only the disabled amazon option is in the list
  3. press down arrow
    Another crash in findindex

Yeah, this is the same edge case that this PR should be fixing. Is it not fixed by this pr?

@oddvernes
Copy link
Collaborator

Yeah, this is the same edge case that this PR should be fixing. Is it not fixed by this pr?

My mistake, the error was in the story 🤦‍♂️ item.trend === '📉' should be item?.trend === '📉' on line 354

Copy link
Collaborator

@oddvernes oddvernes left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants