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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 6, 2025 1:33 pm, Dominik Csapak wrote:
> 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)

I had that initially..

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

but didn't do it cause of this - if it's okay to drop Copy here, then
yes, it's probably easier to do it that way, also makes it less of a
breaking change!

> 
>>   }
>>   
>> -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