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

[Dominik Csapak <d.csapak@proxmox.com>]

On 8/25/25 12:56 PM, Stefan Hanreich wrote:
> 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?
> 

we simply print the string for now if we can't parse it at all
but yes, we probably want to have a generic upid parsing that can parse
all 4 upids we have:

RemoteUpid with pbs upid
RemoteUpid with pve upid
plain pve upid
plain pbs upid



for pve vs pbs we already have a helper in proxmox-yew-comp, but imho
this should either be a separate crate (since we'll want this in pdm
server too) or should live in proxmox-schema (or similar)

for now i think this is ok, especially this is how we do
it for all other views too (if we can't parse print the upid itself)
>> 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