Re: [pve-devel] [PATCH pve-xtermjs 1/1] xtermjs: add support for remote node shells via PDM

[Dominik Csapak <d.csapak@proxmox.com>]

some nits inline, no hard feelings for any of them

On 11/5/25 3:13 PM, Fabian Grünbichler wrote:
> if a remote name and type is specified, adapt the API endpoint base url
> accordingly, and do not send the authentication line, since there is no PDM
> termproxy that handles it. instead, PDM will generate and inject the
> authentication line when proxying the websocket connection to the termproxy on
> the remote.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   xterm.js/src/main.js | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/xterm.js/src/main.js b/xterm.js/src/main.js
> index 902a1c3..897540f 100644
> --- a/xterm.js/src/main.js
> +++ b/xterm.js/src/main.js
> @@ -19,6 +19,8 @@ var term,
>       state = states.start,
>       starttime = new Date();
>   
> +var remote = getQueryParameter('remote');
> +var remote_type = getQueryParameter('remote-type');

nit: we probably should const/let here instead of var
(does not make a difference but it's a nicer behavior
in js)

>   var type = getQueryParameter('console');
>   var vmid = getQueryParameter('vmid');
>   var vmname = getQueryParameter('vmname');
> @@ -175,7 +177,11 @@ function createTerminal() {
>       protocol = (location.protocol === 'https:') ? 'wss://' : 'ws://';
>   
>       var params = {};
> -    var url = '/nodes/' + nodename;
> +    var url = '';
> +    if (remote !== undefined && remote !== "") {

nit: this would probably be more correct and shorter by doing

if (remote) {

(e.g. remote === null would be valid here)

if we *really* want to be safe about the contents it should be

if (typeof remote === 'string' && remote)

> +        url += '/' + remote_type + '/remotes/' + remote;

nit: i'd prefer interpolated strings in new code

url += `/${remote_type}/remotes/${remote}`;

> +    }
> +    url += '/nodes/' + nodename;

same here

>       switch (type) {
>   	case 'kvm':
>   	    url += '/qemu/' + vmid;
> @@ -252,7 +258,10 @@ function runTerminal() {
>   	}, 250);
>       });
>   
> -    socket.send(PVE.UserName + ':' + ticket + "\n");
> +    // for remote sessions, this line needs to be sent by PDM
> +    if (remote === undefined || remote === "") {

same hint as above should probably be

if (!remote) {

or

if (typeof remote !== 'string' || !remote) {

> +        socket.send(PVE.UserName + ':' + ticket + "\n");
> +    }
>   }
>   
>   function getLxcStatus(callback) {



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel