Re: [pdm-devel] [PATCH datacenter-manager v3 4/8] ui: refactor remote upid formatter

[Stefan Hanreich <s.hanreich@proxmox.com>]

one comment inline

On 8/25/25 10:10 AM, Dominik Csapak wrote:
> since that is often the same (aside from the option to render the
> remote), and we'll want to reuse that even further.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  ui/src/remotes/tasks.rs | 19 ++++---------------
>  ui/src/tasks.rs         | 23 ++++++++++++++++++++++-
>  ui/src/top_nav_bar.rs   | 18 +++---------------
>  3 files changed, 29 insertions(+), 31 deletions(-)
> 
> diff --git a/ui/src/remotes/tasks.rs b/ui/src/remotes/tasks.rs
> index 3398647..138b899 100644
> --- a/ui/src/remotes/tasks.rs
> +++ b/ui/src/remotes/tasks.rs
> @@ -7,12 +7,9 @@ use yew::{
>  };
>  
>  use pdm_api_types::RemoteUpid;
> -use pdm_client::types::PveUpid;
>  
>  use proxmox_yew_comp::{
> -    common_api_types::TaskListItem,
> -    utils::{format_task_description, format_upid, render_epoch_short},
> -    TaskViewer, Tasks,
> +    common_api_types::TaskListItem, utils::render_epoch_short, TaskViewer, Tasks,
>  };
>  use pwt::{
>      css::{FlexFit, JustifyContent},
> @@ -24,6 +21,8 @@ use pwt::{
>      },
>  };
>  
> +use crate::tasks::format_optional_remote_upid;
> +
>  #[derive(PartialEq, Properties)]
>  pub struct RemoteTaskList;
>  impl RemoteTaskList {
> @@ -77,17 +76,7 @@ fn columns() -> Rc<Vec<DataTableHeader<TaskListItem>>> {
>          DataTableColumn::new(tr!("Description"))
>              .flex(4)
>              .render(move |item: &TaskListItem| {
> -                if let Ok(remote_upid) = item.upid.parse::<RemoteUpid>() {
> -                    match remote_upid.upid.parse::<PveUpid>() {
> -                        Ok(upid) => {
> -                            format_task_description(&upid.worker_type, upid.worker_id.as_deref())
> -                        }
> -                        Err(_) => format_upid(&remote_upid.upid),
> -                    }
> -                } else {
> -                    format_upid(&item.upid)
> -                }
> -                .into()
> +                format_optional_remote_upid(&item.upid, false).into()
>              })
>              .into(),
>          DataTableColumn::new(tr!("Status"))
> diff --git a/ui/src/tasks.rs b/ui/src/tasks.rs
> index 6aa202a..0e7899c 100644
> --- a/ui/src/tasks.rs
> +++ b/ui/src/tasks.rs
> @@ -1,6 +1,9 @@
> -use proxmox_yew_comp::utils::register_task_description;
> +use proxmox_yew_comp::utils::{format_task_description, format_upid, register_task_description};
>  use pwt::tr;
>  
> +use pdm_api_types::RemoteUpid;
> +use pdm_client::types::PveUpid;
> +
>  pub fn register_pve_tasks() {
>      register_task_description("qmstart", ("VM", tr!("Start")));
>      register_task_description("acmedeactivate", ("ACME Account", tr!("Deactivate")));
> @@ -99,3 +102,21 @@ pub fn register_pve_tasks() {
>      register_task_description("zfscreate", (tr!("ZFS Storage"), tr!("Create")));
>      register_task_description("zfsremove", ("ZFS Pool", tr!("Remove")));
>  }
> +
> +/// Format a UPID that is either [`RemoteUpid`] or a [`UPID`]
> +/// If it's a [`RemoteUpid`], prefixes it with the remote name
> +pub fn format_optional_remote_upid(upid: &str, include_remote: bool) -> String {
> +    if let Ok(remote_upid) = upid.parse::<RemoteUpid>() {
> +        let description = match remote_upid.upid.parse::<PveUpid>() {
> +            Ok(upid) => format_task_description(&upid.worker_type, upid.worker_id.as_deref()),
> +            Err(_) => format_upid(&remote_upid.upid),
> +        };
> +        if include_remote {
> +            format!("{} - {}", remote_upid.remote(), description)
> +        } else {
> +            description
> +        }
> +    } else {
> +        format_upid(&upid)
> +    }
> +}

Maybe some error handling if the &str is not a valid Upid? Maybe we want
an enum / Either<RemoteUpid, PveUpid> type that holds both RemoteUpid /
PveUpid in the future?

> diff --git a/ui/src/top_nav_bar.rs b/ui/src/top_nav_bar.rs
> index 78aace5..069e831 100644
> --- a/ui/src/top_nav_bar.rs
> +++ b/ui/src/top_nav_bar.rs
> @@ -13,15 +13,15 @@ use pwt::state::{Loader, Theme, ThemeObserver};
>  use pwt::widget::{Button, Container, Row, ThemeModeSelector, Tooltip};
>  
>  use proxmox_yew_comp::common_api_types::TaskListItem;
> -use proxmox_yew_comp::utils::{format_task_description, format_upid, set_location_href};
> +use proxmox_yew_comp::utils::set_location_href;
>  use proxmox_yew_comp::RunningTasksButton;
>  use proxmox_yew_comp::{http_get, HelpButton, LanguageDialog, TaskViewer, ThemeDialog};
>  
>  use pwt_macros::builder;
>  
>  use pdm_api_types::RemoteUpid;
> -use pdm_client::types::PveUpid;
>  
> +use crate::tasks::format_optional_remote_upid;
>  use crate::widget::SearchBox;
>  
>  #[derive(Deserialize)]
> @@ -207,19 +207,7 @@ impl Component for PdmTopNavBar {
>                              }),
>                      ])
>                      .render(|item: &TaskListItem| {
> -                        if let Ok(remote_upid) = (&item.upid).parse::<RemoteUpid>() {
> -                            let description = match remote_upid.upid.parse::<PveUpid>() {
> -                                Ok(upid) => format_task_description(
> -                                    &upid.worker_type,
> -                                    upid.worker_id.as_deref(),
> -                                ),
> -                                Err(_) => format_upid(&remote_upid.upid),
> -                            };
> -                            format!("{} - {}", remote_upid.remote(), description)
> -                        } else {
> -                            format_upid(&item.upid)
> -                        }
> -                        .into()
> +                        format_optional_remote_upid(&item.upid, true).into()
>                      }),
>              );
>  



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