-
Notifications
You must be signed in to change notification settings - Fork 185
refactor: show connection lost message when starting machine #2282
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds WebSocket connection monitoring and reconnection functionality to the machine startup page. When a WebSocket connection is lost, the system now displays a warning message with a reconnect button to allow users to re-establish the connection without reloading the page.
Changes:
- Added connection lost alert component to the run request page
- Implemented WebSocket onclose handlers to detect disconnections
- Added subscribe_all function to enable manual reconnection
- Enhanced button styling for the port reopening feature
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| templates/main/run_request.html.ep | Includes the connection_lost partial component and updates button styling for the reopen ports button |
| public/js/ravada.js | Adds WebSocket onclose handlers, implements subscribe_all reconnection function, and stores id_request for reconnection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > | ||
| <div class="jumbotron" ng-cloak=""> | ||
|
|
||
| %= include "/main/connection_lost" |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The include path uses a leading slash "/main/connection_lost" which is inconsistent with how includes are used elsewhere in the codebase. Other templates use relative paths without leading slashes (e.g., 'main/connection_lost' as seen in admin_machines.html.ep). While this might work, it's inconsistent with the project's conventions and could cause issues depending on the template engine configuration.
| %= include "/main/connection_lost" | |
| %= include 'main/connection_lost' |
| ws.onclose = function() { | ||
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); | ||
| }; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 5-second delay before showing the connection lost message could create a confusing user experience. If the WebSocket closes immediately when the page loads or during normal operation, users won't see any feedback for 5 seconds. Additionally, if the user clicks the reconnect button while this timeout is pending, the timeout is not cancelled, which could result in the connection lost message reappearing even after successful reconnection. Consider storing the timeout promise and cancelling it when reconnecting or when a new connection is established.
| }; | ||
|
|
||
| ws.onmessage = function(event) { | ||
| var data = JSON.parse(event.data); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for JSON.parse(). If the server sends malformed JSON data, the JSON.parse() call will throw an exception that is not caught, which will break the WebSocket message handler and potentially leave the application in an inconsistent state. Consider wrapping JSON.parse() in a try-catch block and logging or handling parsing errors appropriately.
| var data = JSON.parse(event.data); | |
| var data; | |
| try { | |
| data = JSON.parse(event.data); | |
| } catch (e) { | |
| if (window && window.console && window.console.error) { | |
| console.error("Failed to parse WebSocket message data:", e, event.data); | |
| } | |
| return; | |
| } |
| }; | ||
|
|
||
| ws.onmessage = function(event) { | ||
| var data = JSON.parse(event.data); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for JSON.parse(). If the server sends malformed JSON data, the JSON.parse() call will throw an exception that is not caught, which will break the WebSocket message handler and potentially leave the application in an inconsistent state. Consider wrapping JSON.parse() in a try-catch block and logging or handling parsing errors appropriately.
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The nested use of $scope.$apply() and $timeout() is unnecessary and could cause errors. The $timeout service already wraps its callback in $apply(), so explicitly calling $apply() around $timeout() creates a nested digest cycle which can lead to "$digest already in progress" errors. The $scope.$apply() wrapper should be removed, keeping only the $timeout() call.
| $scope.$apply(function() { | |
| $timeout(function() { | |
| $scope.ws_connection_lost = true; | |
| }, 5*1000); | |
| }); | |
| $timeout(function() { | |
| $scope.ws_connection_lost = true; | |
| }, 5*1000); |
| ws.onclose = function() { | ||
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onclose handler will be triggered even for normal WebSocket closures (e.g., when the page is navigated away or when connection is successfully closed). This means the "connection lost" message will always appear after 5 seconds when leaving the page or when the WebSocket is intentionally closed. Consider checking the close event's code or wasClean property to distinguish between abnormal and normal closures, or add a flag to track intentional disconnections.
| ws.onclose = function() { | |
| $scope.$apply(function() { | |
| $timeout(function() { | |
| $scope.ws_connection_lost = true; | |
| }, 5*1000); | |
| }); | |
| ws.onclose = function(event) { | |
| // Only treat the closure as "connection lost" if it was not clean. | |
| // This avoids showing the message on normal, intentional closures. | |
| if (!event || event.wasClean === false) { | |
| $scope.$apply(function() { | |
| $timeout(function() { | |
| $scope.ws_connection_lost = true; | |
| }, 5*1000); | |
| }); | |
| } |
| var ws = new WebSocket(url); | ||
| ws.onopen = function(event) { ws.send('request/'+id_request) }; | ||
|
|
||
| ws.onclose = function() { | ||
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); | ||
| }; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for the WebSocket connection. If the WebSocket fails to connect (e.g., due to network issues), there's no onerror handler to manage the error or notify the user. Consider adding a ws.onerror handler that sets ws_connection_lost to true or provides appropriate error feedback.
| }, 5*1000); | ||
| }); | ||
| }; | ||
|
|
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for the WebSocket connection. If the WebSocket fails to connect (e.g., due to network issues), there's no onerror handler to manage the error or notify the user. Consider adding a ws.onerror handler that sets ws_connection_lost to true or provides appropriate error feedback.
| ws.onerror = function(event) { | |
| $scope.$apply(function() { | |
| $timeout(function() { | |
| $scope.ws_connection_lost = true; | |
| }, 5*1000); | |
| }); | |
| }; |
| ws.onclose = function() { | ||
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); | ||
| }; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 5-second delay before showing the connection lost message could create a confusing user experience. If the WebSocket closes immediately when the page loads or during normal operation, users won't see any feedback for 5 seconds. Additionally, if the user clicks the reconnect button while this timeout is pending, the timeout is not cancelled, which could result in the connection lost message reappearing even after successful reconnection. Consider storing the timeout promise and cancelling it when reconnecting or when a new connection is established.
| ws.onclose = function() { | ||
| $scope.$apply(function() { | ||
| $timeout(function() { | ||
| $scope.ws_connection_lost = true; | ||
| }, 5*1000); | ||
| }); | ||
| }; |
Copilot
AI
Jan 16, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onclose handler will be triggered even for normal WebSocket closures (e.g., when the page is navigated away or when connection is successfully closed). This means the "connection lost" message will always appear after 5 seconds when leaving the page or when the WebSocket is intentionally closed. Consider checking the close event's code or wasClean property to distinguish between abnormal and normal closures, or add a flag to track intentional disconnections.
Check for web service disconnection and show button to recconnect