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

Rewrite queue page with hooks #263

Merged
merged 14 commits into from
Apr 9, 2019
Merged

Rewrite queue page with hooks #263

merged 14 commits into from
Apr 9, 2019

Conversation

nwalters512
Copy link
Contributor

@nwalters512 nwalters512 commented Apr 8, 2019

My hope is that this rewrite will help eliminate the 404s we've been seeing at random. My best guess (or, hunch, really) as to why it's happening is a race condition of some kind, which hooks are better at avoiding. We also explicitly maintain our own loading state in the component. Again, this is just a hunch, and we'll have to see if the anecdotal reports of 404s stop after this.

Turns out my initial hunch was totally wrong. Eddie figured out how to reproduce the 404 bug:

  • View a queue on Safari on an iPhone
  • Lock the device
  • Unlock the device

It appears as though Safari kills socket connections when Safari isn't in the foreground. When that would happen, we'd get an error event, which (due to my bad error handling) would cause another error. That error would get picked up by React and rendered as our custom error page by Next.js. Unfortunately, our custom error page didn't work well for this use case, because it would default to showing a misleading 404 if a status code wasn't present on the error (which it wasn't, in this case).

So, this PR does a few things to address these issues:

  • Properly handles websocket errors
  • Reports websocket state to users (reconnecting, connected, failed)
  • Renders appropriate error message for non-HTTP-status errors

Given the context of the original problem, would be awesome if people have iOS devices to test with.

Also fixes or enhances a few other things:

  • Fixes a regression introduced in Improve security for queue settings #233 where the ids of activeStaff models weren't being included in the "get queue" response; adds test to prevent future regressions
  • Changes how course name is displayed on queue pages, which now serves as a link to the appropriate course
  • Limits socket reconnection attempts to 5
  • Explicitly handle socket authentication failures separately from normal socket errors.

@vercel
Copy link

vercel bot commented Apr 8, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@vercel vercel bot temporarily deployed to staging April 9, 2019 00:01 Inactive
Copy link
Member

@james9909 james9909 left a comment

Choose a reason for hiding this comment

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

Add a changelog entry, and we should be good to go!

@nwalters512
Copy link
Contributor Author

Also tested this on Now with Eddie's iPad - it correctly reports that the socket is disconnected, then connecting, then connected again after the device is awoken from sleep.

@nwalters512 nwalters512 merged commit 85a0e12 into master Apr 9, 2019
@nwalters512 nwalters512 deleted the fix-queue-page-loading branch April 9, 2019 17:01
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.

2 participants