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/websocket connection check #47

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

Conversation

Nissa9902
Copy link
Collaborator

@Nissa9902 Nissa9902 commented Nov 6, 2024

Fixes: #40

What was changed:
The WebSocket connection handling was updated to ensure data streaming only happens when the WebSocket connection is in the "CONNECTED" state. Additionally, the user interface now provides real-time feedback about the WebSocket connection status using color-coded notifications (green for connected, red for error, yellow for disconnected).

Why was it changed:
This update ensures that data streaming occurs only when the WebSocket is properly connected, which improves the robustness and reliability of the application. It also provides clear feedback to the user about the connection status, helping them understand when there are issues with the WebSocket connection.

How was it changed:

Introduced a state variable (isWebSocketConnected) to track the connection status of the WebSocket.
Updated WebSocket event handlers (onopen, onclose, onerror) to modify the connection status and notify users of connection changes.
The UI was updated to display color-coded status messages, providing clear visual feedback about the WebSocket state.
Ensured that data streaming and communication only take place when the WebSocket is in the "CONNECTED" state.

@Nissa9902 Nissa9902 self-assigned this Nov 6, 2024
@@ -55,10 +64,26 @@ function App() {
<p>Left Foot: {stepTime.targetZones.left.min} - {stepTime.targetZones.left.max}</p>
<p>Right Foot: {stepTime.targetZones.right.min} - {stepTime.targetZones.right.max}</p>
<p>Average Target Zone: {stepTime.targetZones.average}</p>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to have this just be as a console message or alert rather than a visualization on the page because it could affect other views. Additionally, it's at the bottom of the page so it might not been seen by the client anyway based on the view. We could leave it as is, just something to consider.

Copy link
Collaborator

@colinseper colinseper left a comment

Choose a reason for hiding this comment

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

The code looks good, just consider the change I listed as a comment and decide whether or not you'd want to keep it as a

tag or an alert/console message.

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.

Nicely done!
in the next sprint, let's remember to initiate an automatic reconnection from the frontend when streaming stops, automatic reload, and if these fail then we can notify the client that something is wrong.

This is because with your current implementation, a break in the system will stop the streaming but does not break the socket connection, so all the backend needs to start sending data again is a little trigger from the frontend.

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.

Handle data streaming when connection closes from either end
4 participants