Skip to content

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.

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

Open
frankiejol wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: show connection lost message when starting machine #2282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions public/js/ravada.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1597,10 +1597,19 @@
}
}
$scope.subscribe_request= function(url, id_request) {
$scope.id_request = id_request;
already_subscribed_to_domain = false;
var ws = new WebSocket(url);
ws.onopen = function(event) { ws.send('request/'+id_request) };

ws.onclose = function() {
Comment on lines 1601 to +1605
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The WebSocket instance created in subscribe_request is not stored in a variable accessible outside the function. This prevents proper cleanup when reconnecting, which means calling subscribe_all() will create a new WebSocket without closing the old one, potentially leading to memory leaks and duplicate message handlers. Consider storing the WebSocket instance (e.g., in $scope.ws_request) and closing it before creating a new connection in the subscribe_all function.

Suggested change
already_subscribed_to_domain = false;
var ws = new WebSocket(url);
ws.onopen = function(event) { ws.send('request/'+id_request) };
ws.onclose = function() {
already_subscribed_to_domain = false;
// Close any existing WebSocket before creating a new one to avoid
// multiple active connections and duplicate handlers.
if ($scope.ws_request) {
try {
$scope.ws_request.close();
} catch (e) {
}
}
var ws = new WebSocket(url);
// Store the WebSocket instance on the scope so it can be managed elsewhere.
$scope.ws_request = ws;
ws.onopen = function(event) { ws.send('request/'+id_request) };
ws.onclose = function() {
// Clear the stored reference on close.
$scope.ws_request = null;

Copilot uses AI. Check for mistakes.
$scope.$apply(function() {
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);
});
Comment on lines +1606 to +1610
Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
$scope.$apply(function() {
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);
});
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);

Copilot uses AI. Check for mistakes.
};
Comment on lines +1605 to +1611
Copy link

Copilot AI Jan 16, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines 1602 to +1611
Copy link

Copilot AI Jan 16, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +1605 to +1611
Copy link

Copilot AI Jan 16, 2026

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.

Copilot uses AI. Check for mistakes.

ws.onmessage = function(event) {
var data = JSON.parse(event.data);
Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
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;
}

Copilot uses AI. Check for mistakes.
$scope.$apply(function () {
Expand All @@ -1616,6 +1625,7 @@
}
}
}

$scope.refresh_ports = function(url, id_domain ) {
var id_request = $scope.request.id;
$http.post('/request/open_exposed_ports/'
Expand All @@ -1639,10 +1649,25 @@
});
});
}

$scope.subscribe_all = function(url) {
$scope.ws_connection_lost = false;
$scope.subscribe_request(url, $scope.id_request);
};

$scope.subscribe_domain_info= function(url, id_domain) {
already_subscribed_to_domain = true;
var ws = new WebSocket(url);
Copy link

Copilot AI Jan 16, 2026

Choose a reason for hiding this comment

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

The WebSocket instance created in subscribe_domain_info is not stored in a variable accessible outside the function. This prevents proper cleanup and could result in multiple active WebSocket connections if the function is called multiple times, leading to memory leaks and duplicate message handlers. Consider storing the WebSocket instance (e.g., in $scope.ws_domain) and closing any existing connection before creating a new one.

Copilot uses AI. Check for mistakes.
ws.onopen = function(event) { ws.send('machine_info/'+id_domain) };

ws.onclose = function() {
$scope.$apply(function() {
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);
});
Comment on lines +1664 to +1668
Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
$scope.$apply(function() {
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);
});
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);

Copilot uses AI. Check for mistakes.
Comment on lines +1663 to +1668
Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
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);
});
}

Copilot uses AI. Check for mistakes.
};
Comment on lines +1663 to +1669
Copy link

Copilot AI Jan 16, 2026

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.

Copilot uses AI. Check for mistakes.

Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
ws.onerror = function(event) {
$scope.$apply(function() {
$timeout(function() {
$scope.ws_connection_lost = true;
}, 5*1000);
});
};

Copilot uses AI. Check for mistakes.
ws.onmessage = function(event) {
var data = JSON.parse(event.data);
Copy link

Copilot AI Jan 16, 2026

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.

Copilot uses AI. Check for mistakes.
$scope.$apply(function () {
Expand Down
6 changes: 3 additions & 3 deletions templates/main/run_request.html.ep
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
ng-init="subscribe_request('<%= url_for('ws_subscribe')->to_abs %>',<%= $request->id %>);auto_view=<%= $auto_view %>;timeout=<%= $timeout %>"
>
<div class="jumbotron" ng-cloak="">

%= include "/main/connection_lost"
Copy link

Copilot AI Jan 16, 2026

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.

Suggested change
%= include "/main/connection_lost"
%= include 'main/connection_lost'

Copilot uses AI. Check for mistakes.
<div class="card-header">
<div>
<div style="display: inline-block">
Expand Down Expand Up @@ -74,8 +74,8 @@
ng-show="domain.ports.length">
<h3 ng-show="domain.ports.length"><%=l 'Open ports' %>
</h3>
<div>
<button
<div class="ml-4">
<button class="btn btn-light border border-dark"
ng-show="request_open_ports && domain.requests==0"
ng-click="reload_ports()"
><%=l 'reopen' %></button>
Expand Down
Loading