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.

Sign up for GitHub

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

Jump to bottom

Quickly clicking multiple status filters does not toggle all of them #2728

Closed
1 task done
andycochran opened this issue Nov 5, 2024 · 9 comments
Closed
1 task done

Quickly clicking multiple status filters does not toggle all of them #2728

andycochran opened this issue Nov 5, 2024 · 9 comments
Assignees
@doug-s-nava

Comments

@andycochran
Copy link
Collaborator

andycochran commented Nov 5, 2024 •
edited
Loading

Summary

When you very quickly click several status filters, not all of them are toggled. I am wondering if there’s a race condition or a debounce causing a problem here. If you click slowly, this is not an issue.

Trying to recreate this also sometimes leads to an error. Once you get this error, the URL and UI fall out of sync (clicking statuses change the query params, but the checkboxes do not toggle):

Reproducibility

I tried multiple times and saw the bug intermittently

How to reproduce the bug

From a fresh page load, quickly click all 4 status filters

Browser

Chrome

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mxk0 mxk0 moved this from Icebox to Todo in Simpler.Grants.gov Product Backlog Nov 7, 2024
@acouch
Copy link
Collaborator

acouch commented Nov 7, 2024

@andycochran can you take a video? I can't recreate this.

@andycochran
Copy link
Collaborator Author

https://www.loom.com/share/0ba7844114804ce990f14182d68bea95

@doug-s-nava
Copy link
Collaborator

dug in a little, and from what I can tell this is a matter of a "mousedown" or "keydown" not triggering the "change" event that is necessary to update the UI as it's currently written. Should be able to fix this by specifically binding to "mousedown", "keydown" and "touchstart"

@doug-s-nava
Copy link
Collaborator

After a bit more research, I'm not sure this is something we can solve at all easily. It's looking like an onChange handler is going to fire once the value of an input has changed, so firing anything prior to that will not reliably pick up the change in value unless we are actively changing the value ourselves more quickly. We could theoretically do that but it'd be potentially tricky.

FWIW Some vague research into behavior on other sites (ebay, zappos, etc.) shows similar issues.

@andycochran
Copy link
Collaborator Author

andycochran commented Nov 12, 2024 •
edited
Loading

Okay, let's not address the too-quick clicking for now then. I don't think it's a huge bug. And there is clear UI feedback when an element does/doesn't toggle. If a user comes across this bug, they can simply toggle the element again, which should happen more slowly.

Do we have any ideas what might be causing the "We're sorry" error?

@doug-s-nava
Copy link
Collaborator

doug-s-nava commented Nov 12, 2024 •
edited
Loading

Unable to reproduce that error locally. I was able to reproduce on DEV error by clicking around as fast as I could for a while. The Next server spits out a 500 with "Unexpected token '<', \"<html>\r\n<h\"... is not valid JSON",. This looks like either the API or Next sending back an error page rather than the expected JSON.

I am unable to find any corresponding 500 from the API, though given that the Next server is throwing from the API request function I expect that the API would be involved. It's possible that Next is throwing the error on some kind of rate limit, given that each time you click you're sending a new request out to Next, and then onto the API. The fact I can't reproduce locally likely points to it being related to the network not being able to / not wanting to handle so many requests so quickly. If this is an issue I bet it will turn up in our load testing.

Image

@andycochran
Copy link
Collaborator Author

Should we revisit our debounce method? I'm not sure how it's working exactly, but might be worth checking.

cc @mxk0 are you okay with closing this as "won't fix" for now? We can always reopen.

@doug-s-nava
Copy link
Collaborator

Connected my local env to the PROD API briefly to try and reproduce the 500 and failed. Without deploying something out to dev with some extra debugging code I think this is going to be hard to track down at this point.

As far as debouncing goes, I don't see any code that is using a debounce at this point. The app still includes a useDebounce hook library, but it is unused as far as I can tell. @acouch can you give me a second pair of eyes on that state of affairs? Believe we can go ahead and pull out the debounce library and add it back if we want it later? #2936

@doug-s-nava doug-s-nava moved this from In Progress to Done in Simpler.Grants.gov Product Backlog Nov 25, 2024
@doug-s-nava
Copy link
Collaborator

As suggested by @andycochran and @mxk0 - closing as won't fix for now, but perhaps to be revisited later if this proves to be an enduring issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@doug-s-nava doug-s-nava

Labels
None yet
Projects
Simpler.Grants.gov Product Backlog
Status: Done
Milestone
No milestone
Development

No branches or pull requests
3 participants
@andycochran @acouch @doug-s-nava