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/select all crashes #3428

Merged
merged 2 commits into from
May 7, 2024

Conversation

mhwaage
Copy link
Contributor

@mhwaage mhwaage commented May 7, 2024

Removes a seemingly unnecessary useEffect, which resolves an issue where choosing select all would crash when clicked twice in rapid succession. (#3426)

This resolves a crash that occurs when clicking select all 'too' quickly
which otherwise can trigger an infinite loop inside this useEffect.
@mhwaage
Copy link
Contributor Author

mhwaage commented May 7, 2024

Some important caveats:

  1. I was not able to create a test case that reproduces the issue, hence no tests are added for preventing regressions.
  2. The useEffect being removed is the one where the code was crashing. There is no obvious reason why the useEffect should be in place, and I couldn't see any rationale for it via git blame.
  3. The removal does not cause any test failures, but I don't know how good the test coverage is.
  4. The removal resolves the crash reported in When clicking Autocomplete's Select All really fast, everything crashes. #3426 when I install the package locally into that same code base.

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.

While it is a bit unnerving to not know what problem this particular bit of code was supposed to solve (guessing in the past, selectedOptions sent into the Autocomplete on load did not get set initially, or something like that, perhaps a bug with downshift 🤷‍♂️), I am unable to find any problems with removing it, so lets do it 👍

@oddvernes oddvernes merged commit e8a13ca into equinor:develop May 7, 2024
4 of 6 checks passed
mhwaage added a commit to mhwaage/design-system that referenced this pull request Jul 29, 2024
* Move toggleAllSelected so it is clear which variables it is using

* Remove deeply suspicious useEffect

This resolves a crash that occurs when clicking select all 'too' quickly
which otherwise can trigger an infinite loop inside this useEffect.
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.

None yet

2 participants