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

[Hannes Laimer <h.laimer@proxmox.com>]

small comment inline, I'll change the rest for v3. Thanks for the
review!

On 11/7/25 13:27, Lukas Wagner wrote:
> 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
> 

we have
```
     type Message = FirewallMsg;
```
in the component later, we'd leak the type if it was private

>> +
>> +#[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")?
> 

no :)

>> +                        _ => 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
> 
> 



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