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

Adding a reconnect button for websocket disconnection #70

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

JackMcPhillips1543
Copy link
Collaborator

Fixes #63

What was changed:
The disconnected banner was changed to add a reconnect button

Why was this changed:
This was changed because the users had no way to reconnect the websocket if the connection was dropped, rendering the app useless.

How was this changed:
I added a button to appear when the websocket disconnected. I changed how the webosckets connect by creating a setup websocket function that can be called more than once

@JackMcPhillips1543 JackMcPhillips1543 self-assigned this Feb 3, 2025
@mmehta2669 mmehta2669 self-requested a review February 3, 2025 22:29
Copy link
Collaborator

@mmehta2669 mmehta2669 left a comment

Choose a reason for hiding this comment

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

Overall your code looks good and functional. It has passed all the tests looks to work as intend. The only minor problem was the removal of some comments that maybe useful to have in the code

git-voo
git-voo previously approved these changes Feb 3, 2025
Copy link
Collaborator

@git-voo git-voo left a comment

Choose a reason for hiding this comment

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

Good job, Jack!
This looks good to go. As @mmehta2669 pointed out, it'd be nice to have those comments as some form of mild documentation.

One point to note for future implementation is reconnecting disruption that shuts down the server. In such a case we'd need to differentiate between a closed socket connection and an entirely closed server.

websocket.current.close();
console.log("WebSocket connection closed manually.");
}

websocket.current = new WebSocket("ws://localhost:8000/ws");

Choose a reason for hiding this comment

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

Hard-coding the address is generally a bad idea. Not sure if that's how the project handles it in other places, but a better approach is to define a configuration file and place the server address there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will put this into a separate issue, the fix generates a bunch of connection problems.

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.

Reconnect Websocket - Feeback from User Testing
4 participants