Copilot commented on code in PR #12187:
URL: https://github.com/apache/cloudstack/pull/12187#discussion_r2588271030
##########
systemvm/agent/noVNC/app/ui.js:
##########
@@ -1740,6 +1753,51 @@ const UI = {
UI.idleControlbar();
},
+ _sendKeyUp(keysym, code) {
Review Comment:
Missing null check for `UI.rfb` before calling `sendKey()`. If the RFB
connection is not initialized, this will throw a runtime error. Add a check
like `if (!UI.rfb) return;` at the beginning of the method, consistent with
other methods in this file (e.g., lines 1386, 1402, 1444).
```suggestion
_sendKeyUp(keysym, code) {
if (!UI.rfb) return;
```
##########
systemvm/agent/noVNC/app/ui.js:
##########
@@ -1740,6 +1753,51 @@ const UI = {
UI.idleControlbar();
},
+ _sendKeyUp(keysym, code) {
+ UI.rfb.sendKey(keysym, code, false);
+ },
+
+ // Release a single modifier key if it's pressed
+ _releaseModifierKey(keyName) {
+ const keyConfig = UI._modifierKeys[keyName];
+ if (!keyConfig) return false;
+
+ const btn = document.getElementById(keyConfig.buttonId);
+ if (!btn || !btn.classList.contains("noVNC_selected")) {
+ return false;
+ }
+
+ UI._sendKeyUp(keyConfig.keysym, keyConfig.code);
+ btn.classList.remove("noVNC_selected");
+ return true;
+ },
Review Comment:
Missing null check for `UI.rfb` before calling `_sendKeyUp()`. Although
`_releaseAllModifierKeys()` checks for `UI.rfb` existence, this method can be
called independently and should be defensive. Add a check like `if (!UI.rfb)
return false;` at the beginning of the method.
##########
systemvm/agent/noVNC/app/ui.js:
##########
@@ -1740,6 +1753,51 @@ const UI = {
UI.idleControlbar();
},
+ _sendKeyUp(keysym, code) {
+ UI.rfb.sendKey(keysym, code, false);
+ },
+
+ // Release a single modifier key if it's pressed
+ _releaseModifierKey(keyName) {
+ const keyConfig = UI._modifierKeys[keyName];
+ if (!keyConfig) return false;
+
+ const btn = document.getElementById(keyConfig.buttonId);
+ if (!btn || !btn.classList.contains("noVNC_selected")) {
+ return false;
+ }
+
+ UI._sendKeyUp(keyConfig.keysym, keyConfig.code);
+ btn.classList.remove("noVNC_selected");
+ return true;
+ },
+
+ // Release all currently pressed modifier keys
+ _releaseAllModifierKeys() {
+ if (!UI.rfb || UI.rfb._rfbConnectionState !== 'connected') {
+ return false;
+ }
+
+ let keysReleased = false;
+
+ // Release all modifier keys
+ for (const keyName in UI._modifierKeys) {
+ if (UI._releaseModifierKey(keyName)) {
+ keysReleased = true;
+ }
+ }
+
+ // Also check RFB's internal shift state (it tracks this separately)
+ if (UI.rfb._shiftPressed) {
+ const shiftConfig = UI._modifierKeys.shift;
+ UI._sendKeyUp(shiftConfig.keysym, shiftConfig.code);
+ keysReleased = true;
+ }
+
+ return keysReleased;
+ },
+
+
Review Comment:
[nitpick] Extra blank line. Remove one of the blank lines to maintain
consistency with the rest of the codebase, which uses single blank lines
between methods.
```suggestion
```
##########
systemvm/agent/noVNC/app/ui.js:
##########
@@ -1740,6 +1753,51 @@ const UI = {
UI.idleControlbar();
},
+ _sendKeyUp(keysym, code) {
+ UI.rfb.sendKey(keysym, code, false);
+ },
+
+ // Release a single modifier key if it's pressed
+ _releaseModifierKey(keyName) {
+ const keyConfig = UI._modifierKeys[keyName];
+ if (!keyConfig) return false;
+
+ const btn = document.getElementById(keyConfig.buttonId);
+ if (!btn || !btn.classList.contains("noVNC_selected")) {
+ return false;
+ }
+
+ UI._sendKeyUp(keyConfig.keysym, keyConfig.code);
+ btn.classList.remove("noVNC_selected");
+ return true;
+ },
+
+ // Release all currently pressed modifier keys
+ _releaseAllModifierKeys() {
+ if (!UI.rfb || UI.rfb._rfbConnectionState !== 'connected') {
Review Comment:
Accessing the private property `_rfbConnectionState` directly breaks
encapsulation. The RFB class may change this internal implementation in the
future. Consider using a public API method to check the connection state, or
checking if `UI.connected` is true instead, which is already used throughout
the codebase to track connection status.
```suggestion
if (!UI.connected) {
```
##########
systemvm/agent/noVNC/app/ui.js:
##########
@@ -1740,6 +1753,51 @@ const UI = {
UI.idleControlbar();
},
+ _sendKeyUp(keysym, code) {
+ UI.rfb.sendKey(keysym, code, false);
+ },
+
+ // Release a single modifier key if it's pressed
+ _releaseModifierKey(keyName) {
+ const keyConfig = UI._modifierKeys[keyName];
+ if (!keyConfig) return false;
+
+ const btn = document.getElementById(keyConfig.buttonId);
+ if (!btn || !btn.classList.contains("noVNC_selected")) {
+ return false;
+ }
+
+ UI._sendKeyUp(keyConfig.keysym, keyConfig.code);
+ btn.classList.remove("noVNC_selected");
+ return true;
+ },
+
+ // Release all currently pressed modifier keys
+ _releaseAllModifierKeys() {
+ if (!UI.rfb || UI.rfb._rfbConnectionState !== 'connected') {
+ return false;
+ }
+
+ let keysReleased = false;
+
+ // Release all modifier keys
+ for (const keyName in UI._modifierKeys) {
+ if (UI._releaseModifierKey(keyName)) {
+ keysReleased = true;
+ }
+ }
+
+ // Also check RFB's internal shift state (it tracks this separately)
+ if (UI.rfb._shiftPressed) {
+ const shiftConfig = UI._modifierKeys.shift;
+ UI._sendKeyUp(shiftConfig.keysym, shiftConfig.code);
+ keysReleased = true;
+ }
Review Comment:
Accessing the private property `_shiftPressed` directly breaks
encapsulation. This internal RFB state may not be reliable or could change in
future versions. Additionally, there's already logic to release shift via the
button state check above, making this additional check potentially redundant.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
