Re: [pve-devel] [PATCH proxmox-yew-comp 1/1] xtermjs: add remote support

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

some comments inline

On 11/5/25 3:14 PM, Fabian Grünbichler wrote:
> by defining a new ConsoleType and adding a remote_name property/parameter.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>   src/apt_package_manager.rs |  9 ++++++---
>   src/xtermjs.rs             | 38 +++++++++++++++++++++++++++++++++-----
>   2 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/src/apt_package_manager.rs b/src/apt_package_manager.rs
> index 2e2d01a..0b392be 100644
> --- a/src/apt_package_manager.rs
> +++ b/src/apt_package_manager.rs
> @@ -202,9 +202,12 @@ impl LoadableComponent for ProxmoxAptPackageManager {
>           let on_upgrade = props.on_upgrade.clone();
>           let on_upgrade = move |_| match &on_upgrade {
>               Some(on_upgrade) => on_upgrade.emit(()),
> -            None => {
> -                XTermJs::open_xterm_js_viewer(crate::ConsoleType::UpgradeShell, "localhost", false)
> -            }
> +            None => XTermJs::open_xterm_js_viewer(
> +                crate::ConsoleType::UpgradeShell,
> +                None,
> +                "localhost",
> +                false,
> +            ),
>           };
>   
>           let toolbar = Toolbar::new()
> diff --git a/src/xtermjs.rs b/src/xtermjs.rs
> index 4eb464d..36b5379 100644
> --- a/src/xtermjs.rs
> +++ b/src/xtermjs.rs
> @@ -17,6 +17,11 @@ pub struct XTermJs {
>       #[prop_or_default]
>       pub key: Option<Key>,
>   
> +    #[prop_or_default]
> +    /// The remote name, if this is a remote node shell
> +    #[builder(IntoPropValue, into_prop_value)]
> +    pub remote_name: Option<AttrValue>,
> +
>       #[prop_or("localhost".into())]
>       #[builder(IntoPropValue, into_prop_value)]
>       /// The node name.
> @@ -46,8 +51,13 @@ impl XTermJs {
>   
>       // FIXME: separate noVNC and xterm.js, this is not a nice interface!
>       /// Open a new terminal window.
> -    pub fn open_xterm_js_viewer(console_type: ConsoleType, node_name: &str, vnc: bool) {
> -        let url = xtermjs_url(console_type, node_name, vnc);
> +    pub fn open_xterm_js_viewer(
> +        console_type: ConsoleType,
> +        remote_name: Option<&str>,
> +        node_name: &str,
> +        vnc: bool,
> +    ) {
> +        let url = xtermjs_url(console_type, remote_name, node_name, vnc);
>           let target = "_blank";
>           let features =
>               "toolbar=no,location=no,status=no,menubar=no,resizable=yes,width=800,height=420";
> @@ -72,14 +82,20 @@ pub enum ConsoleType {
>       LXC(u64),
>       UpgradeShell,
>       LoginShell,
> +    RemotePveLoginShell,

would it be possible to have the remote name as property here like this:

RemotePveLoginShell(AttrValue) / RemotePveLoginShell(String)

? (similar to how we add the vmid to the kvm/lxc variants)

we'd lose the `Copy` trait, but then we could... [continued below]

>   }
>   
> -fn xtermjs_url(console_type: ConsoleType, node_name: &str, vnc: bool) -> String {
> +fn xtermjs_url(
> +    console_type: ConsoleType,
> +    remote_name: Option<&str>,
> +    node_name: &str,
> +    vnc: bool,
> +) -> String {
>       let console = match console_type {
>           ConsoleType::KVM(_vmid) => "kvm",
>           ConsoleType::LXC(_vmid) => "lxc",
>           ConsoleType::UpgradeShell => "upgrade",
> -        ConsoleType::LoginShell => "shell",
> +        ConsoleType::LoginShell | ConsoleType::RemotePveLoginShell => "shell",
>       };
>   
>       let mut param = json!({
> @@ -105,6 +121,13 @@ fn xtermjs_url(console_type: ConsoleType, node_name: &str, vnc: bool) -> String
>           ConsoleType::LoginShell => {
>               param["cmd"] = "login".into();
>           }
> +        ConsoleType::RemotePveLoginShell => {
> +            param["cmd"] = "login".into();
> +            param["remote-type"] = "pve".into();
> +            param["remote"] = remote_name
> +                .expect("RemotePveLoginShell requires remote name")
> +                .into();
> +        }

omit the 'expect' here. In general having unwrap/expect in the gui code 
is not a good idea, since it crashes the whole ui...

even if it's not possible with this patch to trigger it,
the interface can be wrongly used too easily imho


it would also save a parameter in most functions here



>       }
>   
>       format!("?{}", json_object_to_query(param).unwrap())
> @@ -122,7 +145,12 @@ impl Component for ProxmoxXTermJs {
>   
>       fn view(&self, ctx: &Context<Self>) -> Html {
>           let props = ctx.props();
> -        let url = xtermjs_url(props.console_type, &props.node_name, props.vnc);
> +        let url = xtermjs_url(
> +            props.console_type,
> +            props.remote_name.as_deref(),
> +            &props.node_name,
> +            props.vnc,
> +        );
>           html! {<iframe class="pwt-flex-fit" src={format!("/{url}")}/>}
>       }
>   }



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