From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 77B421FF141 for ; Mon, 30 Mar 2026 15:49:00 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id F31D2324A2; Mon, 30 Mar 2026 15:49:26 +0200 (CEST) Message-ID: <66064577-9ace-4443-b9bc-cb1a70a68043@proxmox.com> Date: Mon, 30 Mar 2026 15:48:51 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH yew-comp 1/3] task (viewer): add task download button To: Shannon Sterz References: <20260330125320.507302-1-d.csapak@proxmox.com> <20260330125320.507302-2-d.csapak@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1774878477196 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.458 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 1 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: E2OCC2UKVXJX5K44Z5DTPA7LRIZZNFHL X-Message-ID-Hash: E2OCC2UKVXJX5K44Z5DTPA7LRIZZNFHL X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: yew-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Yew framework devel list at Proxmox List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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::() { >> + 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 `` 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::>(url, Some(param)).await; >> + match resp { >> + Ok(resp) => { >> + let string = resp >> + .data >> + .into_iter() >> + .map(|entry| entry.t) >> + .collect::>() >> + .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>>>, >> + >> + #[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 { >