Re: [PATCH yew-comp 1/3] task (viewer): add task download button

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

On 3/30/26 3:35 PM, Shannon Sterz wrote:
> On Mon Mar 30, 2026 at 2:53 PM CEST, Dominik Csapak wrote:
[snip]
>> @@ -273,18 +297,94 @@ impl PwtTaskViewer {
>>           let active = self.active;
>>           let link = ctx.link();
>>
>> -        let toolbar = Toolbar::new().class("pwt-border-bottom").with_child(
>> -            Button::new(tr!("Stop"))
>> -                .disabled(!active)
>> -                .onclick(link.callback(|_| Msg::StopTask)),
>> -        );
>> -
>>           let url = format!(
>>               "{}/{}/log",
>>               props.base_url,
>>               percent_encode_component(&task_id),
>>           );
>>
>> +        let filename = match props.task_id.parse::<ProxmoxUpid>() {
>> +            Ok(upid) => {
>> +                format!(
>> +                    "task-{}-{}-{}.log",
>> +                    upid.node,
>> +                    upid.worker_type,
>> +                    epoch_to_rfc3339_utc(upid.starttime).unwrap_or(upid.starttime.to_string())
>> +                )
>> +            }
>> +            Err(_) => format!("task-{}.log", props.task_id),
>> +        };
>> +
>> +        let download_btn: Html = if props.download_api {
>> +            Container::from_tag("a")
>> +                .attribute("disabled", active.then_some(""))
>> +                .attribute("target", "_blank")
>> +                .attribute("href", format!("/api2/extjs{url}?download=1"))
>> +                .attribute("filename", filename)
>> +                .with_child(
>> +                    Button::new(tr!("Download"))
>> +                        .disabled(active)
>> +                        .icon_class("fa fa-download"),
>> +                )
>> +                .into()
> 
> is there a reason you construct a new `<a>` element as a container here
> instead of using the `download_as_file` helper in an `on_activate`
> callback for the button?
> 

if we have a static link, there is no need to create an 'a' element
on the fly and clicking it programmatically. Since browsers use
heuristics to detect 'user-initiated' actions to determine if things
are allowed (e.g. auto-clicking on links) such helpers should be used
sparingly.

a static link is better expressed with native browser methods such as
an a element that the user clicks. (the button inside is just for
cosmetics in this case)

e.g. if one would send a message in on_activate and call the helper in
the update method, this might not work as intended since the browser
can't detect that it was user-initiated.

so my intent was to only use it when absolutely necessary.

it's also what we do in PDM to download the report (though there we
use a data url)

the idea here was also that once all task log endpoints support the
download api parameter, we can remove the button below again,
and the helper too.

do you see any advantage with using the helper in the first case too?

>> +        } else {
>> +            Button::new(tr!("Download"))
>> +                .icon_class("fa fa-download")
>> +                .disabled(active)
>> +                .on_activate({
>> +                    let url = url.clone();
>> +                    let link = link.clone();
>> +                    let async_pool = self.async_pool.clone();
>> +                    move |_| {
>> +                        let url = url.clone();
>> +                        let filename = filename.clone();
>> +                        let link = link.clone();
>> +                        async_pool.spawn(async move {
>> +                            let param = json!({
>> +                                "start": 0,
>> +                                "limit": 0,
>> +                            });
>> +                            let resp =
>> +                                crate::http_get_full::<Vec<LogEntry>>(url, Some(param)).await;
>> +                            match resp {
>> +                                Ok(resp) => {
>> +                                    let string = resp
>> +                                        .data
>> +                                        .into_iter()
>> +                                        .map(|entry| entry.t)
>> +                                        .collect::<Vec<_>>()
>> +                                        .join("\n");
>> +
>> +                                    if let Err(err) = download_as_file(
>> +                                        &format!(
>> +                                            "data:text/plain;charset=utf-8,{}",
>> +                                            percent_encode_component(&string)
>> +                                        ),
>> +                                        &filename,
>> +                                    ) {
>> +                                        link.send_message(Msg::DownloadErr(Some(err.to_string())))
>> +                                    }
>> +                                }
>> +                                Err(err) => {
>> +                                    link.send_message(Msg::DownloadErr(Some(err.to_string())))
>> +                                }
>> +                            }
>> +                        });
>> +                    }
>> +                })
>> +                .into()
>> +        };
>> +
>> +        let toolbar = Toolbar::new()
>> +            .class("pwt-border-bottom")
>> +            .with_child(
>> +                Button::new(tr!("Stop"))
>> +                    .disabled(!active)
>> +                    .onclick(link.callback(|_| Msg::StopTask)),
>> +            )
>> +            .with_flex_spacer()
>> +            .with_child(download_btn);
>> +
>>           Column::new()
>>               .class("pwt-flex-fit")
>>               .with_child(toolbar)
>> diff --git a/src/tasks.rs b/src/tasks.rs
>> index b49de98..fb3ed39 100644
>> --- a/src/tasks.rs
>> +++ b/src/tasks.rs
>> @@ -78,6 +78,11 @@ pub struct Tasks {
>>       #[prop_or_default]
>>       /// An optional column configuration that overwrites the default one.
>>       pub columns: Option<Rc<Vec<DataTableHeader<TaskListItem>>>>,
>> +
>> +    #[prop_or(false)]
>> +    #[builder(IntoPropValue, into_prop_value)]
>> +    /// Use the 'download' API parameter for downloading task logs.
>> +    pub download_api: bool,
>>   }
>>
>>   impl Default for Tasks {
>> @@ -493,6 +498,7 @@ impl LoadableComponent for ProxmoxTasks {
>>           match view_state {
>>               ViewDialog::TaskViewer => {
>>                   let mut dialog = TaskViewer::new(&*selected_key)
>> +                    .download_api(props.download_api)
>>                       .endtime(selected_item.endtime)
>>                       .on_close(ctx.link().change_view_callback(|_| None));
>>                   if let Some(base_url) = &props.base_url {
>