Re: [RFC PATCH datacenter-manager 2/3] ui: acl: list granular level permission paths for views

["Shannon Sterz" <s.sterz@proxmox.com>]

some comments in-line.

On Wed Mar 25, 2026 at 4:35 PM CET, Shan Shaji wrote:
> Previously, users were unable to assign permissions to individual views.
> Resolved this by loading the views list and extending permission paths
> to include specific view routes.
>
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
>  .../configuration/permission_path_selector.rs | 80 +++++++++++++++----
>  1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/ui/src/configuration/permission_path_selector.rs b/ui/src/configuration/permission_path_selector.rs
> index 59bdabb..a0fec86 100644
> --- a/ui/src/configuration/permission_path_selector.rs
> +++ b/ui/src/configuration/permission_path_selector.rs
> @@ -1,12 +1,15 @@
>  use std::rc::Rc;
>
> +use anyhow::Error;
>  use yew::html::IntoPropValue;
>
> -use pwt::prelude::*;
>  use pwt::widget::form::Combobox;
> +use pwt::{prelude::*, AsyncPool};
>
>  use pwt_macros::{builder, widget};
>
> +use crate::pdm_client;
> +
>  static PREDEFINED_PATHS: &[&str] = &[
>      "/",
>      "/access",
> @@ -44,33 +47,78 @@ impl PermissionPathSelector {
>      }
>  }
>
> -enum Msg {}
> +enum Msg {
> +    Prefetched(Vec<String>),
> +    PrefetchFailed,
> +}
>
>  struct PdmPermissionPathSelector {
>      items: Rc<Vec<AttrValue>>,
> +    async_pool: AsyncPool,
>  }
>
> -impl PdmPermissionPathSelector {}
> +impl PdmPermissionPathSelector {
> +    async fn get_view_paths() -> Result<Vec<String>, Error> {
> +        let views = pdm_client().list_views().await?;
> +        let paths: Vec<String> = views
> +            .iter()
> +            .map(|cfg| format!("/view/{}", cfg.id))
> +            .collect();
> +        Ok(paths)
> +    }
> +
> +    async fn get_paths() -> Result<Vec<String>, Error> {
> +        let paths = Self::get_view_paths().await?;
> +        Ok(paths)
> +    }
> +}
>
>  impl Component for PdmPermissionPathSelector {
>      type Message = Msg;
>      type Properties = PermissionPathSelector;
>
> -    fn create(_ctx: &Context<Self>) -> Self {
> -        // TODO: fetch resources & remotes from the backend to improve the pre-defined selection of
> -        // acl paths
> -        Self {
> -            items: Rc::new(
> -                PREDEFINED_PATHS
> -                    .iter()
> -                    .map(|i| AttrValue::from(*i))
> -                    .collect(),
> -            ),
> -        }
> +    fn create(ctx: &Context<Self>) -> Self {
> +        let base_items: Vec<AttrValue> = PREDEFINED_PATHS
> +            .iter()
> +            .map(|i| AttrValue::from(*i))
> +            .collect();
> +
> +        let this = Self {
> +            items: Rc::new(base_items),
> +            async_pool: AsyncPool::new(),
> +        };
> +
> +        let link = ctx.link().clone();
> +        this.async_pool.spawn(async move {
> +            let paths = Self::get_paths().await;
> +            match paths {
> +                Ok(paths) => {
> +                    link.send_message(Msg::Prefetched(paths));
> +                }
> +                Err(_) => {
> +                    link.send_message(Msg::PrefetchFailed);
> +                }

this is minor, but at least for me cargo fmt will also accept the
following formatting:

            match paths {
                Ok(paths) => link.send_message(Msg::Prefetched(paths)),
                Err(_) => link.send_message(Msg::PrefetchFailed),
            }

imo that's a little neater.

> +            }
> +        });

im generally not too big of a fan of this "this" pattern in rust and
here there is no need for it, you can simply do:

    let async_pool = AsyncPool::new();
    async_pool.spawn(..);
    Self {
        items: Rc::new(..),
        async_pool,
    }

you may need to change `async_pool` in the struct to `_async_pool`
because then this member is never directly accessed.

> +
> +        this
>      }
>
> -    fn update(&mut self, _ctx: &Context<Self>, _msg: Self::Message) -> bool {
> -        false
> +    fn update(&mut self, _ctx: &Context<Self>, msg: Self::Message) -> bool {
> +        match msg {
> +            Msg::Prefetched(paths) => {
> +                let mut items: Vec<String> = Vec::with_capacity(self.items.len() + paths.len());
> +
> +                items.extend(self.items.iter().map(|item| item.to_string()));
> +                items.extend(paths.into_iter());
> +                items.sort_by(|a, b| a.to_lowercase().cmp(&b.to_lowercase()));
> +
> +                self.items = Rc::new(items.into_iter().map(AttrValue::from).collect());

this could be expressed more neatly as:

```
            Msg::Prefetched(paths) => {
                let items = Rc::make_mut(&mut self.items);
                items.extend(paths.into_iter().map(AttrValue::from));
                items.sort_by_key(|k| k.to_lowercase());
                true
            }
```

this has a couple of advantages:

* we don't move the original items from `AttrValue` to a `String` back
  to `AttrValue`, saving on allocations.
* `Rc::make_mut()` will only clone if there are other pointers to the
  same allocation. since the is an `Rc::clone` in the view method below,
  this probably won't actually impact much here from what i can tell,
  but it still seems sensible to me to only force a new `Rc<Vec>` if
  absolutely needed.
* uses `sort_by_key` instead of `sort_by` is more concise and also
  recommended by clippy.

> +
> +                true
> +            }
> +            Msg::PrefetchFailed => false,
> +        }
>      }
>
>      fn view(&self, ctx: &Context<Self>) -> Html {