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

Fix bug where search was performed on undefined string causing crash #4403

Merged
merged 1 commit into from Jan 22, 2024

Conversation

norbye
Copy link
Member

@norbye norbye commented Jan 18, 2024

Description

The check was for null values, but after the refactor the value was set to undefined - which is correct typewise, but fucked up the search function.
Removed the null-value check as a null-value should not be allowed with the current type (string | undefined).

Result

No crash

Testing

  • I have thoroughly tested my changes.

Please describe what and how the changes have been tested, and provide instructions to reproduce if necessary.


Resolves ABA-746

@norbye norbye added review-needed Pull requests that need review bug-fix Pull requests that fix a bug labels Jan 18, 2024
Copy link

linear bot commented Jan 18, 2024

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Hahah this was driving me nuts! Thanks for just fixing❤️

@norbye norbye added approved Pull requests that have been approved ready-to-merge Pull requests that have been approved and are ready to be merged and removed review-needed Pull requests that need review labels Jan 18, 2024
@ivarnakken
Copy link
Member

ivarnakken commented Jan 19, 2024

@norbye
Have you looked at the code in ReactionPickerContent.tsx as well? Shouldn't the type and the two checks be wrong here as well now?
EDIT: I now see that the useMemo returns null, so this is actually correct. I misread.

But, this check in ReactionPicker.tsx seems wrong, no?

@ivarnakken ivarnakken added the question Pull requests with an unresolved question label Jan 19, 2024
@norbye
Copy link
Member Author

norbye commented Jan 19, 2024

@norbye Have you looked at the code in ReactionPickerContent.tsx as well? Shouldn't the type and the two checks be wrong here as well now? EDIT: I now see that the useMemo returns null, so this is actually correct. I misread.

ReactionPickerContent.tsx doesn't have access to searchString, so that should be good - at least from any changes in this PR.

But, this check in ReactionPicker.tsx seems wrong, no?

You're absolutely right, will fix:) while we're on it, it always checks for both undefined and empty string, so I might as well just set it to default to an empty string and as such the type won't allow undefined.

Would've been so nice if the linter picked up comparisons between invalid values

@github-actions github-actions bot added the review-needed Pull requests that need review label Jan 19, 2024
@ivarnakken ivarnakken removed question Pull requests with an unresolved question review-needed Pull requests that need review labels Jan 19, 2024
@github-actions github-actions bot added the review-needed Pull requests that need review label Jan 20, 2024
@norbye norbye removed the review-needed Pull requests that need review label Jan 20, 2024
@norbye norbye enabled auto-merge January 20, 2024 07:03
@norbye norbye removed the review-needed Pull requests that need review label Jan 22, 2024
@norbye norbye merged commit bff44ea into master Jan 22, 2024
3 of 4 checks passed
@norbye norbye deleted the reaction-buggie branch January 22, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Pull requests that have been approved bug-fix Pull requests that fix a bug ready-to-merge Pull requests that have been approved and are ready to be merged
Projects
None yet
3 participants