Re: [pdm-devel] [PATCH proxmox-yew-comp v2 4/4] firewall: add rules table

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

Looking great, I think this new style should work pretty well.

General note, since this is a component library, the public structs/fns
should have doc comments.

Some further notes inline.

On Wed Nov 5, 2025 at 5:35 PM CET, Hannes Laimer wrote:
> Displays the list of firewall rules, this is read-only currently, so it
> doesn't include any buttons for editing or adding rules.
>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/firewall/mod.rs   |   3 +
>  src/firewall/rules.rs | 253 ++++++++++++++++++++++++++++++++++++++++++
>  src/lib.rs            |   2 +-
>  3 files changed, 257 insertions(+), 1 deletion(-)
>  create mode 100644 src/firewall/rules.rs
>
> diff --git a/src/firewall/mod.rs b/src/firewall/mod.rs
> index 379b958..8cc4977 100644
> --- a/src/firewall/mod.rs
> +++ b/src/firewall/mod.rs
> @@ -4,5 +4,8 @@ pub use context::FirewallContext;
>  mod options_edit;
>  pub use options_edit::EditFirewallOptions;
>  
> +mod rules;
> +pub use rules::FirewallRules;
> +
>  mod log_ratelimit_field;
>  pub use log_ratelimit_field::LogRatelimitField;
> diff --git a/src/firewall/rules.rs b/src/firewall/rules.rs
> new file mode 100644
> index 0000000..0add7db
> --- /dev/null
> +++ b/src/firewall/rules.rs
> @@ -0,0 +1,253 @@
> +use std::rc::Rc;
> +
> +use yew::html::{IntoEventCallback, IntoPropValue};
> +use yew::virtual_dom::{Key, VComp, VNode};
> +
> +use pwt::prelude::*;
> +use pwt::state::{Loader, LoaderState, SharedStateObserver, Store};
> +use pwt::widget::data_table::{DataTable, DataTableColumn, DataTableHeader};
> +use pwt::widget::Container;
> +use pwt_macros::builder;
> +
> +use super::context::FirewallContext;
> +
> +#[derive(Clone, PartialEq, Properties)]
> +#[builder]
> +pub struct FirewallRules {
> +    #[builder(IntoPropValue, into_prop_value)]
> +    pub context: FirewallContext,
> +
> +    #[builder_cb(IntoEventCallback, into_event_callback, ())]
> +    #[prop_or_default]
> +    pub on_close: Option<Callback<()>>,
> +}
> +
> +impl FirewallRules {
> +    pub fn cluster(remote: impl Into<AttrValue>) -> Self {
> +        yew::props!(Self {
> +            context: FirewallContext::cluster(remote),
> +        })
> +    }
> +
> +    pub fn node(remote: impl Into<AttrValue>, node: impl Into<AttrValue>) -> Self {
> +        yew::props!(Self {
> +            context: FirewallContext::node(remote, node),
> +        })
> +    }
> +
> +    pub fn guest(
> +        remote: impl Into<AttrValue>,
> +        node: impl Into<AttrValue>,
> +        vmid: u64,
> +        vmtype: impl Into<AttrValue>,
> +    ) -> Self {
> +        yew::props!(Self {
> +            context: FirewallContext::guest(remote, node, vmid, vmtype),
> +        })
> +    }
> +}
> +
> +pub enum FirewallMsg {
> +    DataChange,
> +}

I *think* this one does not need to be public

> +
> +#[doc(hidden)]
> +pub struct ProxmoxFirewallRules {

Along with this one

> +    store: Store<pve_api_types::ListFirewallRules>,
> +    loader: Loader<Vec<pve_api_types::ListFirewallRules>>,
> +    _listener: SharedStateObserver<LoaderState<Vec<pve_api_types::ListFirewallRules>>>,
> +}
> +
> +fn pill(text: impl Into<AttrValue>) -> Container {
> +    Container::from_tag("span")
> +        .style("display", "inline-block")
> +        .style("margin", "0 1px")
> +        .style("background-color", "var(--pwt-color-neutral-container)")
> +        .style("color", "var(--pwt-color-on-neutral-container)")
> +        .style("border-radius", "var(--pwt-button-corner-shape)")
> +        .style("padding-inline", "var(--pwt-spacer-2)")
> +        .with_child(text.into())
> +}
> +
> +fn format_firewall_rule(rule: &pve_api_types::ListFirewallRules) -> Html {
> +    let mut parts: Vec<VNode> = Vec::new();
> +
> +    if let Some(iface) = &rule.iface {
> +        parts.push(pill(format!("iface: {iface}")).into());
> +    }
> +
> +    if let Some(macro_name) = &rule.r#macro {
> +        parts.push(pill(format!("macro: {}", macro_name)).into());

You can inline the variable into the format! here - same for the
format!s below.

> +    }
> +
> +    if let Some(proto) = &rule.proto {
> +        let mut proto_str = proto.to_uppercase();
> +        if proto.eq("icmp") || proto.eq("icmpv6") || proto.eq("ipv6-icmp") {

It's more idiomatic to use `==` instead of .eq

> +            if let Some(icmp_type) = &rule.icmp_type {
> +                proto_str = format!("{}, {}", proto_str, icmp_type);
> +            }
> +        }
> +        parts.push(pill(tr!("proto: {}", proto_str)).into());

Any reason why this one specifically should be translated?
I'd be tempted to not translate any of these abbreviations for new until
somebody complains about it, but maybe others have different opinions
here.

> +    }
> +
> +    match (&rule.source, &rule.sport) {
> +        (Some(src), Some(sport)) => {
> +            parts.push(pill(format!("src: {}, port: {}", src, sport)).into());
> +        }
> +        (Some(src), None) => {
> +            parts.push(pill(format!("src: {}", src)).into());
> +        }
> +        (None, Some(sport)) => {
> +            parts.push(pill(format!("src: any, port: {}", sport)).into());

I know it comes from the API type, but if possible, use something like
`source_port` or `src_port` as variable names.

> +        }
> +        _ => {}
> +    }
> +
> +    match (&rule.dest, &rule.dport) {
> +        (Some(dst), Some(dport)) => {
> +            parts.push(pill(format!("dest: {}:, port: {}", dst, dport)).into());
                                                ^
superfluous ':' here

> +        }
> +        (Some(dst), None) => {
> +            parts.push(pill(format!("dest: {}", dst)).into());
> +        }
> +        (None, Some(dport)) => {
> +            parts.push(pill(format!("dest: any, port: {}", dport)).into());
> +        }
> +        _ => {}
> +    }
> +
> +    if parts.is_empty() {
> +        return "-".into();

Personally I'd just leave the row completely empty if there are no
rules, but that's just a matter of personal preference, no need to
change if you think this is better.

> +    }
> +
> +    parts
> +        .into_iter()
> +        .enumerate()
> +        .flat_map(|(i, part)| {
> +            if i == 0 {
> +                vec![part]
> +            } else {
> +                vec![" ".into(), part]
> +            }
> +        })
> +        .collect::<Html>()
> +}
> +
> +impl ProxmoxFirewallRules {
> +    fn update_data(&mut self) {
> +        if let Some(Ok(data)) = &self.loader.read().data {
> +            self.store.set_data((**data).clone());
> +        }
> +    }
> +
> +    fn columns() -> Rc<Vec<DataTableHeader<pve_api_types::ListFirewallRules>>> {
> +        Rc::new(vec![
> +            DataTableColumn::new("")
> +                .width("30px")
> +                .justify("right")
> +                .show_menu(false)
> +                .resizable(false)
> +                .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.pos})
> +                .into(),
> +            DataTableColumn::new(tr!("On"))
> +                .width("40px")
> +                .justify("center")
> +                .resizable(false)
> +                .render(
> +                    |rule: &pve_api_types::ListFirewallRules| match rule.enable {
> +                        Some(1) => html! {<i class="fa fa-check"></i>},
> +                        Some(0) | None => html! {<i class="fa fa fa-minus"></i>},

Any reason why you use the html! macro here and not Fa::new("check") or
Fa::new("minus")?

> +                        _ => html! {"-"},
> +                    },
> +                )
> +                .into(),
> +            DataTableColumn::new(tr!("Type"))
> +                .width("80px")
> +                .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.ty})
> +                .into(),
> +            DataTableColumn::new(tr!("Action"))
> +                .width("100px")
> +                .render(|rule: &pve_api_types::ListFirewallRules| html! {&rule.action})
> +                .into(),
> +            DataTableColumn::new(tr!("Rule"))
> +                .flex(1)
> +                .render(|rule: &pve_api_types::ListFirewallRules| format_firewall_rule(rule))
> +                .into(),
> +            DataTableColumn::new(tr!("Comment"))
> +                .width("150px")
> +                .render(|rule: &pve_api_types::ListFirewallRules| {
> +                    rule.comment.as_deref().unwrap_or("-").into()
> +                })
> +                .into(),
> +        ])
> +    }
> +}
> +
> +impl Component for ProxmoxFirewallRules {
> +    type Message = FirewallMsg;
> +    type Properties = FirewallRules;
> +
> +    fn create(ctx: &Context<Self>) -> Self {
> +        let props = ctx.props();
> +
> +        let url: AttrValue = props.context.rules_url().into();
> +
> +        let store = Store::with_extract_key(|item: &pve_api_types::ListFirewallRules| {
> +            Key::from(item.pos.to_string())
> +        });
> +
> +        let loader = Loader::new().loader({
> +            let url = url.clone();
> +            move || {
> +                let url = url.clone();
> +                async move { crate::http_get(url.to_string(), None).await }
> +            }
> +        });
> +
> +        let _listener = loader.add_listener(ctx.link().callback(|_| FirewallMsg::DataChange));
> +
> +        loader.load();
> +
> +        let mut me = Self {
> +            store,
> +            loader,
> +            _listener,
> +        };
> +
> +        me.update_data();
> +        me
> +    }
> +
> +    fn update(&mut self, _ctx: &Context<Self>, msg: Self::Message) -> bool {
> +        match msg {
> +            FirewallMsg::DataChange => {
> +                self.update_data();
> +                true
> +            }
> +        }
> +    }
> +
> +    fn view(&self, _ctx: &Context<Self>) -> Html {
> +        self.loader.render(|_data| -> Html {
> +            if self.store.data_len() == 0 {
> +                Container::new()
> +                    .padding(2)
> +                    .with_child(tr!("No firewall rules configured"))
> +                    .into()
> +            } else {
> +                let columns = Self::columns();
> +                DataTable::new(columns, self.store.clone())
> +                    .show_header(true)
> +                    .striped(true)
> +                    .into()
> +            }
> +        })
> +    }
> +}
> +
> +impl From<FirewallRules> for VNode {
> +    fn from(val: FirewallRules) -> Self {
> +        let comp = VComp::new::<ProxmoxFirewallRules>(Rc::new(val), None);
> +        VNode::from(comp)
> +    }
> +}
> diff --git a/src/lib.rs b/src/lib.rs
> index 852d65d..d7d8c7e 100644
> --- a/src/lib.rs
> +++ b/src/lib.rs
> @@ -130,7 +130,7 @@ mod rrd_timeframe_selector;
>  pub use rrd_timeframe_selector::{RRDTimeframe, RRDTimeframeSelector};
>  
>  mod firewall;
> -pub use firewall::{EditFirewallOptions, FirewallContext};
> +pub use firewall::{EditFirewallOptions, FirewallContext, FirewallRules};
>  
>  mod running_tasks;
>  pub use running_tasks::{ProxmoxRunningTasks, RunningTasks};



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