- Improve WebSocket handler utility class #41

marioserrano09 · 2025-12-03T19:40:00Z

No description provided.

…ssion management Signed-off-by: Mario Serrano <[email protected]>

…m.xml Signed-off-by: Mario Serrano <[email protected]>

…on in SchedulerUtil Signed-off-by: Mario Serrano <[email protected]>

Copilot

Pull request overview

This PR improves the WebSocket handler utility class by implementing bidirectional heartbeat support (PING/PONG), refactoring session management, and bumping the framework version to 5.4.10. The changes add server-side heartbeat broadcasting capabilities and improve error handling when sending WebSocket messages.

Key changes:

Added bidirectional PING/PONG heartbeat mechanism between client and server
Refactored WebSocket session management with a new DeskstopWebSocketSession record wrapper
Improved error handling with separate IOException handling and session cleanup
Bumped all module versions from 5.4.9 to 5.4.10

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
zk/src/main/resources/static/dynamia-tools/js/dynamia-tools-ws.js	Increased keep-alive ping interval from 30s to 60s; added server PING handling with PONG response
zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java	Added `broadcastHeartbeat()` method; improved error handling with IOException separation; changed `getHandler()` visibility to public
zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java	Refactored session storage with `DeskstopWebSocketSession` record; added PONG handling; simplified session lookup logic; added `closeSession()` method
integration/src/main/java/tools/dynamia/integration/scheduling/SchedulerUtil.java	Renamed method from `scheduleWithFixedDelay` to `scheduleAtFixedRate` with corrected documentation
Multiple pom.xml files	Version bump from 5.4.9 to 5.4.10 across all modules

Copilot · 2025-12-03T19:45:54Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java

+                LOGGER.error("IO Error sending push command '" + command + "' to Dekstop: " + desktop, e);
+                handler.closeSession(desktop);
            } catch (Exception e) {
                LOGGER.error("Error sending push command '" + command + "' to Dekstop: " + desktop, e);


Spelling error: "Dekstop" should be "Desktop"

Suggested change

LOGGER.error("IO Error sending push command '" + command + "' to Dekstop: " + desktop, e);

handler.closeSession(desktop);

} catch (Exception e) {

LOGGER.error("Error sending push command '" + command + "' to Dekstop: " + desktop, e);

LOGGER.error("IO Error sending push command '" + command + "' to Desktop: " + desktop, e);

handler.closeSession(desktop);

} catch (Exception e) {

LOGGER.error("Error sending push command '" + command + "' to Desktop: " + desktop, e);

Copilot · 2025-12-03T19:45:55Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java

+                LOGGER.error("IO Error sending push command '" + command + "' to Dekstop: " + desktop, e);
+                handler.closeSession(desktop);
            } catch (Exception e) {
                LOGGER.error("Error sending push command '" + command + "' to Dekstop: " + desktop, e);


Spelling error: "Dekstop" should be "Desktop"

Suggested change

LOGGER.error("IO Error sending push command '" + command + "' to Dekstop: " + desktop, e);

handler.closeSession(desktop);

} catch (Exception e) {

LOGGER.error("Error sending push command '" + command + "' to Dekstop: " + desktop, e);

LOGGER.error("IO Error sending push command '" + command + "' to Desktop: " + desktop, e);

handler.closeSession(desktop);

} catch (Exception e) {

LOGGER.error("Error sending push command '" + command + "' to Desktop: " + desktop, e);

Copilot · 2025-12-03T19:45:55Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

-        log("Associating desktop " + desktopId + " with ws session " + session.getId());
-        desktops.put(desktopId, session.getId());
+        log("Associating desktop " + payload + " with ws session " + session.getId());
+        sessions.put(session.getId(), new DeskstopWebSocketSession(payload, session));


Spelling error in record name: "DeskstopWebSocketSession" should be "DesktopWebSocketSession"

Copilot · 2025-12-03T19:45:56Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

-            }
-        }
-        return session;
+    public DeskstopWebSocketSession findSession(Desktop desktop) {


Spelling error in record name: "DeskstopWebSocketSession" should be "DesktopWebSocketSession"

Copilot · 2025-12-03T19:45:56Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

     */
-    Collection<WebSocketSession> getAllSessions() {
-        return sessions.values();
+    public Map<String, DeskstopWebSocketSession> getSessions() {


Spelling error in record name: "DeskstopWebSocketSession" should be "DesktopWebSocketSession"

Copilot · 2025-12-03T19:45:56Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

+        var oldSession = sessions.get(payload);
+        if (oldSession != null) {
+            oldSession.close(CloseStatus.NORMAL);


Potential bug in session replacement logic: When a desktop re-registers, the old session is retrieved using sessions.get(payload) where payload is the desktop ID, but sessions is now a map keyed by session ID, not desktop ID. This will always return null, preventing the cleanup of old sessions when a desktop reconnects. The old logic correctly looked up sessions by desktop ID first, then session ID.

Suggested change

var oldSession = sessions.get(payload);

if (oldSession != null) {

oldSession.close(CloseStatus.NORMAL);

DeskstopWebSocketSession oldSession = null;

String oldSessionId = null;

for (Map.Entry<String, DeskstopWebSocketSession> entry : sessions.entrySet()) {

if (payload.equals(entry.getValue().getDesktopId())) {

oldSession = entry.getValue();

oldSessionId = entry.getKey();

break;

}

}

if (oldSession != null) {

oldSession.close(CloseStatus.NORMAL);

sessions.remove(oldSessionId);

Copilot · 2025-12-03T19:45:57Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java

+        broadcastCommand("PING");
+    }
+
+


[nitpick] API design: Changing the visibility of getHandler() from private to public makes an internal utility method part of the public API without documentation explaining when external code should use it. Consider adding Javadoc to explain the purpose and usage of this method, or evaluate if this visibility change is necessary.

Suggested change

/**

* Returns the global {@link WebSocketGlobalCommandHandler} instance used to manage all WebSocket sessions and commands.

* <p>

* This method allows external code to access the handler for advanced operations, such as querying sessions or sending custom commands.

* Use with caution, as direct manipulation of the handler may affect all connected clients.

*

* @return the global {@link WebSocketGlobalCommandHandler} instance, or {@code null} if not available

*

* Example:

* <pre>{@code

* WebSocketGlobalCommandHandler handler = WebSocketPushSender.getHandler();

* if (handler != null) {

* handler.getSessions().forEach((desktop, session) -> {

* // Custom logic

* });

* }

* }</pre>

*/

Copilot · 2025-12-03T19:45:57Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

-    private final Map<String, String> desktops = new ConcurrentHashMap<>();
-    private final Map<String, WebSocketSession> sessions = new ConcurrentHashMap<>();
+    // Map of active WebSocket sessions by session ID
+    private final Map<String, DeskstopWebSocketSession> sessions = new ConcurrentHashMap<>();


Spelling error in record name: "DeskstopWebSocketSession" should be "DesktopWebSocketSession". This typo appears in the type declaration and affects the entire class structure.

Copilot · 2025-12-03T19:45:57Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketPushSender.java

                } catch (IOException e) {
-                    LOGGER.error("Error sending command " + command + " to WS Session: " + s);
+                    LOGGER.error("IO Error sending command " + command + " to WS Session: " + s, e);
+                    s.close(CloseStatus.NORMAL);


Potential resource leak: When an IOException occurs during broadcast, the session is closed with CloseStatus.NORMAL, but it's not removed from the sessions map. This differs from the pattern in the sendPushCommand method where handler.closeSession(desktop) is called to properly clean up the session. Consider using a similar cleanup approach here to ensure the session is removed from the map.

Suggested change

s.close(CloseStatus.NORMAL);

// Properly clean up the session by removing it from the handler

handler.closeSession(s.getDesktop());

Copilot · 2025-12-03T19:45:58Z

zk/src/main/java/tools/dynamia/zk/websocket/WebSocketGlobalCommandHandler.java

+    public Map<String, DeskstopWebSocketSession> getSessions() {
+        return sessions;


API design: The return type has changed from Collection<WebSocketSession> to Map<String, DeskstopWebSocketSession>, which is a breaking change for any code that calls this method. The new type exposes internal session IDs and the misspelled DeskstopWebSocketSession type. Consider whether this should return an unmodifiable collection or view to prevent external modification of the internal session map.

marioserrano09 added 3 commits December 3, 2025 14:32

feat: Enhance WebSocket handling with improved ping/pong logic and se…

53400a6

…ssion management Signed-off-by: Mario Serrano <[email protected]>

feat: Upgrade DynamiaTools version to 5.4.10 across all modules in po…

0d4a858

…m.xml Signed-off-by: Mario Serrano <[email protected]>

feat: Update scheduling method to use fixed rate for repeated executi…

4fd2e94

…on in SchedulerUtil Signed-off-by: Mario Serrano <[email protected]>

Copilot AI review requested due to automatic review settings December 3, 2025 19:40

marioserrano09 merged commit cd5e14b into main Dec 3, 2025
3 checks passed

Copilot started reviewing on behalf of marioserrano09 December 3, 2025 19:40 View session

Copilot finished reviewing on behalf of marioserrano09 December 3, 2025 19:42

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

- Improve WebSocket handler utility class #41

- Improve WebSocket handler utility class #41

Uh oh!

marioserrano09 commented Dec 3, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Copilot AI Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-        var oldSession = sessions.get(payload);
-        if (oldSession != null) {
-            oldSession.close(CloseStatus.NORMAL);
+        DeskstopWebSocketSession oldSession = null;
+        String oldSessionId = null;
+        for (Map.Entry<String, DeskstopWebSocketSession> entry : sessions.entrySet()) {
+            if (payload.equals(entry.getValue().getDesktopId())) {
+                oldSession = entry.getValue();
+                oldSessionId = entry.getKey();
+                break;
+            }
+        }
+        if (oldSession != null) {
+            oldSession.close(CloseStatus.NORMAL);
+            sessions.remove(oldSessionId);

+    /**
+     * Returns the global {@link WebSocketGlobalCommandHandler} instance used to manage all WebSocket sessions and commands.
+     * <p>
+     * This method allows external code to access the handler for advanced operations, such as querying sessions or sending custom commands.
+     * Use with caution, as direct manipulation of the handler may affect all connected clients.
+     *
+     * @return the global {@link WebSocketGlobalCommandHandler} instance, or {@code null} if not available
+     *
+     * Example:
+     * <pre>{@code
+     * WebSocketGlobalCommandHandler handler = WebSocketPushSender.getHandler();
+     * if (handler != null) {
+     *     handler.getSessions().forEach((desktop, session) -> {
+     *         // Custom logic
+     *     });
+     * }
+     * }</pre>
+     */

	s.close(CloseStatus.NORMAL);
	// Properly clean up the session by removing it from the handler
	handler.closeSession(s.getDesktop());

		public Map<String, DeskstopWebSocketSession> getSessions() {
		return sessions;

- Improve WebSocket handler utility class #41

- Improve WebSocket handler utility class #41

Uh oh!

Conversation

marioserrano09 commented Dec 3, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants