feat(webapp): integrate the RDP and VNC WASM packages by RRRadicalEdward · Pull Request #1329 · Devolutions/devolutions-gateway

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T07:35:00Z

This PR changes the Devolutions Gateway's webapp to integrate the new modular VNC and RDP packages.
Key changes:

Refactored the webapp to use the updated API from iron-remote-desktop
Update the VNC connection form with new specific settings: Enabled Encodings, Disable Cursor, etc.
Update the ARD connection form with Resolutions Quality and Quality Mode settings.
Enhanced the viewer toolbar; added Toggle Cursor Style button and Unicode Keyboard Mode checkbox.

Also, I've tested integration, and all 3 protocols are working as expected.
Here are some demos:

VNC.mp4

RDP.mp4

ARD.mp4

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T07:50:37Z

Benoît Cortier (@CBenoit) Please review when you get a chance

Benoît Cortier (CBenoit)

This the PR is quite big, would you be okay with adding "Copilot" as a reviewer and let it perform a first pass? I think I’m not able to add it myself, could you try on your side?
https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T08:54:49Z

This the PR is quite big, would you be okay with adding "Copilot" as a reviewer and let it perform a first pass?

Yes. Sure

I think I’m not able to add it myself, could you try on your side? https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review

I'm afraid I can't do it either. I can't set a reviewer because of a lack of rights

Benoît Cortier (CBenoit) · 2025-05-13T09:06:22Z

This PR changes the Devolutions Gateway's webapp to integrate the new modular VNC and RDP packages. Key changes:

* Refactored the webapp to use the updated API from `iron-remote-desktop`

* Update the _VNC_ connection form with new specific settings: `Enabled Encodings`, `Disable Cursor`, etc.

* Update the _ARD_ connection form with `Resolutions Quality` and `Quality Mode` settings.

* Enhanced the viewer toolbar; added `Toggle Cursor Style` button and `Unicode Keyboard Mode` checkbox.

Great job!

VNC.mp4

It’s now possible to configure all the codecs, excellent!

A few questions and suggestions:

What about enabling all of them by default? I think most people don’t really care about what is enabled until they want to disable some specific codecs for various reasons, so we should let the negotiation happens with all the codecs by default.
What does "Disable cursor" refers to?
I would prefer we state the flags positively: "Enable cursor" (checked by default) instead of "Disable cursor" (unchecked by default). It’s easier to scan. (I think this is generally true and does not apply only to UI.)
Do we support "non-Unicode input" at all for VNC? I’m pretty sure Unicode input should be hardcoded to "true" for VNC and ARD.

RDP.mp4

IIRC, we do not support Unicode input for RDP yet? This is planned, but until it’s actually implemented, could you hide the checkbox (comment it out with a comment explaining this should be enabled back later).

(Also, this should not be checked by default for RDP in my opinion.)

ARD.mp4

As mentioned above, Unicode input should maybe be hardcoded to "true", with no checkbox.

Benoît Cortier (CBenoit) · 2025-05-13T09:09:55Z

I think I’m not able to add it myself, could you try on your side? https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review

I'm afraid I can't do it either. I can't set a reviewer because of a lack of rights

Maybe because you are not part of the organization. Since I can’t assign it on my side either, it’s likely not possible to request a review at all in this case.

Benoît Cortier (CBenoit) · 2025-05-13T10:44:44Z

-      this.formService.addControlToForm(
-        this.form,
-        'authMode',
+      this.formService.addControlToForm({
+        formGroup: this.form,
+        controlName: 'authMode',
        inputFormData,
-        true,
-        false,
-        SshAuthMode.Username_and_Password,
-      );
+        isRequired: true,
+        defaultValue: SshAuthMode.Username_and_Password,
+      });


praise: I think it’s more readable that way, especially for the booleans 👍 (named parameters)

Benoît Cortier (CBenoit)

I’ve gone through the PR! Many improvements, good job! To be picky: I think it’s better to open smaller PRs for the small refactorings unrelated to the main change, and it’s easier to review all the things separately.

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T11:13:07Z

What does "Disable cursor" refers to?

It disables the Cursor pseudo-encoding which tells the server to draw the mouse cursor into the framebuffer.
The user may want to disable it to improve performance, because it slightly reduces the server's load.

I would prefer we state the flags positively: "Enable cursor" (checked by default) instead of "Disable cursor" (unchecked by default). It’s easier to scan. (I think this is generally true and does not apply only to UI.)

Done.

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T11:14:25Z

Do we support "non-Unicode input" at all for VNC? I’m pretty sure Unicode input should be hardcoded to "true" for VNC and ARD.

Yeah. I removed it for VNC and ARD

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T11:17:50Z

IIRC, we do not support Unicode input for RDP yet? This is planned, but until it’s actually implemented, could you hide the checkbox (comment it out with a comment explaining this should be enabled back later).

(Also, this should not be checked by default for RDP in my opinion.)

I commented it out

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T11:18:33Z

ARD.mp4

As mentioned above, Unicode input should maybe be hardcoded to "true", with no checkbox.

Done

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T11:19:05Z

What about enabling all of them by default? I think most people don’t really care about what is enabled until they want to disable some specific codecs for various reasons, so we should let the negotiation happens with all the codecs by default.

Done

Benoît Cortier (CBenoit) · 2025-05-13T12:54:07Z

IIRC, we do not support Unicode input for RDP yet? This is planned, but until it’s actually implemented, could you hide the checkbox (comment it out with a comment explaining this should be enabled back later).

(Also, this should not be checked by default for RDP in my opinion.)

I commented it out

I'm sorry, you were right: we support it already. My only suggestion is to uncheck it by default (this is the default behavior in the official client too I believe).

Benoît Cortier (CBenoit) · 2025-05-13T15:08:11Z

What does "Disable cursor" refers to?

It disables the Cursor pseudo-encoding which tells the server to draw the mouse cursor into the framebuffer. The user may want to disable it to improve performance, because it slightly reduces the server's load.

Got you! I think that if the server supports it, we may receive cursor images that we handle ourselves, right? And I understand that in this case the performance is not badly impacted.

What about modifying this way?

Checkbox label: Enable cursor pseudo-encoding (a bit, jargony, but specific 🤔)
Add a tooltip: When enabled, the server may render the mouse cursor into the framebuffer, which can affect performance depending on the server capabilities

Alex Yusiuk (RRRadicalEdward) · 2025-05-13T15:45:18Z

IIRC, we do not support Unicode input for RDP yet? This is planned, but until it’s actually implemented, could you hide the checkbox (comment it out with a comment explaining this should be enabled back later).

(Also, this should not be checked by default for RDP in my opinion.)

I commented it out

I'm sorry, you were right: we support it already. My only suggestion is to uncheck it by default (this is the default behavior in the official client too I believe).

Done

Benoît Cortier (CBenoit)

LGTM! Excellent work!

Benoît Cortier (CBenoit) · 2025-05-13T23:31:24Z

Can you address this comment #1329 (comment) in a follow up PR? Thanks!

Alex Yusiuk (RRRadicalEdward) · 2025-05-14T07:14:19Z

Can you address this comment #1329 (comment) in a follow up PR? Thanks!

Done in #1335

feat(webapp): integrate the RDP and VNC WASM packages

122290c

Alex Yusiuk (RRRadicalEdward) commented May 13, 2025

View reviewed changes

Comment thread webapp/src/client/app/modules/web-client/ssh/web-client-ssh.component.ts Outdated

Alex Yusiuk (RRRadicalEdward) commented May 13, 2025

View reviewed changes

Comment thread webapp/src/client/app/modules/web-client/ard/web-client-ard.component.ts

Alex Yusiuk (RRRadicalEdward) commented May 13, 2025

View reviewed changes

Comment thread webapp/tsconfig.json Outdated