Re: [pdm-devel] [PATCH proxmox-datacenter-manager v2 4/4] ui: add firewall status tree

["Lukas Wagner" <l.wagner@proxmox.com>]

Looks good from what I can tell (not a super deep review though), some
notes inline.

There is some small bug that I've found, when you use the << button to
collapse the tree and then expand it again, it is slightly smaller than
before, noticable from the "x out of y rules enabled" text not fitting
the view any more.

Also we need to find some way to handle unavailable remotes more
gracefully. Right now, a single unavailable remote can block the entire
loading progress until that one eventually times out; this is definitely
not great UX-wise. But I think this can also be improved in a future
patch series, IMO no need to block this until then.

On Wed Nov 5, 2025 at 5:35 PM CET, Hannes Laimer wrote:
> +    let mut remote_handle = root.append(remote_entry);
> +    remote_handle.set_expanded(cluster_is_enabled);
> +
> +    for node_status in remote_status.nodes {
> +        let node_name = node_status.node.clone();
> +        let node_firewall_status = node_status.status;
> +
> +        let node_entry = TreeEntry::Node(NodeEntry {
> +            remote: remote_name.clone(),
> +            name: node_name.clone(),
> +            status: node_firewall_status,
> +            masked: !cluster_is_enabled,
> +        });
> +
> +        let mut node_handle = remote_handle.append(node_entry);
> +        node_handle.set_expanded(!node_status.guests.is_empty());
> +
> +        for guest in node_status.guests {
> +            let guest_entry = GuestEntry::new(
> +                guest.clone(),
> +                node_name.clone(),
> +                remote_name.clone(),
> +                !cluster_is_enabled,
> +            );
> +
> +            let tree_entry = match guest.kind {
> +                GuestKind::Lxc => TreeEntry::Guest(guest_entry, GuestKind::Lxc),
> +                GuestKind::Qemu => TreeEntry::Guest(guest_entry, GuestKind::Qemu),
> +            };

This can just be:

            let tree_entry = TreeEntry::Guest(guest_entry, guest.kind);

> +
> +            node_handle.append(tree_entry);
> +        }
> +    }
> +}
> +
> +fn sort_entries(a: &TreeEntry, b: &TreeEntry) -> Ordering {
> +    let rank_a = a.sort_rank();
> +    let rank_b = b.sort_rank();
> +    match rank_a.cmp(&rank_b) {
> +        Ordering::Equal => a.name().cmp(&b.name()),
> +        other => other,
> +    }
> +}
> +
> +pub enum Msg {
> +    DataLoaded {
> +        generation: usize,
> +        data: Vec<pdm_api_types::firewall::RemoteFirewallStatus>,
> +    },
> +    RemoteListChanged,
> +    Reload,
> +    FilterChanged(String),
> +    ScopeChanged(Scope),
> +    RemotesLoaded(Vec<String>),
> +    NodesLoaded {
> +        generation: usize,
> +        nodes: Vec<String>,
> +    },
> +    SelectionChanged(Option<yew::virtual_dom::Key>),
> +    ToggleTreePanel,
> +    Error(FirewallError),
> +    NoOp,
> +}
> +

 [...]

> +
> +fn create_remote_combobox(
> +    ctx: &LoadableComponentContext<FirewallTreeComponent>,
> +    available_remotes: &[String],
> +    options_loading: bool,
> +    current_scope: &Scope,
> +) -> Html {
> +    if options_loading {
> +        return Combobox::new()
> +            .items(Rc::new(vec![]))
> +            .placeholder(tr!("Loading..."))
> +            .disabled(true)
> +            .key("remote-combobox-loading")
> +            .on_change(ctx.link().callback(|_: String| Msg::NoOp))
> +            .into();
> +    }
> +
> +    let items: Vec<yew::AttrValue> = available_remotes
> +        .iter()
> +        .map(|remote| yew::AttrValue::from(remote.clone()))
> +        .collect();
> +
> +    let current_value = current_scope
> +        .remote_name()
> +        .map(|s| yew::AttrValue::from(s.to_string()));
> +
> +    Combobox::new()
> +        .items(Rc::new(items))
> +        .default(current_value)
> +        .placeholder(tr!("All remotes"))

This combobox also shows PBS remotes, I guess they need to be filtered
out somewhere along the way.

> +        .disabled(false)
> +        .key("remote-combobox")
> +        .on_change(ctx.link().callback(move |value: String| {
> +            if value.is_empty() {
> +                Msg::ScopeChanged(Scope::All)
> +            } else {
> +                Msg::ScopeChanged(Scope::Remote { name: value })
> +            }
> +        }))
> +        .into()
> +}
> +



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