[yew-devel] [PATCH yew-comp/yew-widget-toolkit 0/4] fix crash and improve task loading

[yew-devel] [PATCH yew-comp/yew-widget-toolkit 0/4] fix crash and improve task loading

[Dominik Csapak]

fixes #6382, and improves the task loading behavior that I noticed
during the bug fix

proxmox-yew-widget-toolkit:

Dominik Csapak (2):
  fix #6382: widget: data table: prevent duplicate yew keys for rows
  widget: data table: add table scroll callback

 src/widget/data_table/data_table.rs | 22 +++++++++++++++++++++-
 src/widget/data_table/row.rs        |  9 +++++++--
 2 files changed, 28 insertions(+), 3 deletions(-)


proxmox-yew-comp:

Dominik Csapak (2):
  tasks: improve behavior of loading additional tasks
  tasks: don't add entries we already loaded

 src/tasks.rs | 69 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 54 insertions(+), 15 deletions(-)


Summary over all repositories:
  3 files changed, 82 insertions(+), 18 deletions(-)

-- 
Generated by git-murpp 0.8.1


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

[yew-devel] [PATCH yew-widget-toolkit 1/2] fix #6382: widget: data table: prevent duplicate yew keys for rows

[Dominik Csapak]

yews diff algorithm assumes that elements in a list have unique keys, so
it can properly diff the new and old list. Our Store/Datastore does not
guarantee that records have unique keys.

For the data table rows, we used the record keys as yew keys, so in case
we generated duplicate entries in our datastore, we had duplicate keys
for yew, which in turn can panic.

To prevent that for the data table, prefix the record key with the index
into the store, the combination of which must be unique in this list.

Only using the row would not be enough though, otherwise yew would reuse
items even if they're unrelated. e.g. when the whole store content
changes.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/widget/data_table/row.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/widget/data_table/row.rs b/src/widget/data_table/row.rs
index 2770eb9..707ed31 100644
--- a/src/widget/data_table/row.rs
+++ b/src/widget/data_table/row.rs
@@ -80,7 +80,7 @@ impl<T: Clone + PartialEq + 'static> Component for PwtDataTableRow<T> {
         };
 
         let mut row = Container::from_tag("tr")
-            .key(props.record_key.clone())
+            .key(generate_key(&props))
             .attribute("role", "row")
             // aria-rowindex does not work, no firefox support?
             .attribute("aria-rowindex", (props.row_num + 1).to_string())
@@ -189,8 +189,13 @@ impl<T: Clone + PartialEq + 'static> Component for PwtDataTableRow<T> {
 
 impl<T: Clone + PartialEq + 'static> From<DataTableRow<T>> for VNode {
     fn from(val: DataTableRow<T>) -> Self {
-        let key = Some(val.record_key.clone());
+        let key = Some(generate_key(&val));
         let comp = VComp::new::<PwtDataTableRow<T>>(Rc::new(val), key);
         VNode::from(comp)
     }
 }
+
+// generate unique key from rownum + record key, since record key might not be unique
+fn generate_key<T: Clone + PartialEq>(row: &DataTableRow<T>) -> Key {
+    format!("{}-{}", row.row_num, row.record_key).into()
+}
-- 
2.39.5



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

[yew-devel] [PATCH yew-widget-toolkit 2/2] widget: data table: add table scroll callback

[Dominik Csapak]

sometimes it's useful to do things on scrolling, e.g. when wanting to
load additional data when at the end of the table.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/widget/data_table/data_table.rs | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/widget/data_table/data_table.rs b/src/widget/data_table/data_table.rs
index f926955..cb0f781 100644
--- a/src/widget/data_table/data_table.rs
+++ b/src/widget/data_table/data_table.rs
@@ -7,7 +7,7 @@ use derivative::Derivative;
 use gloo_timers::callback::Timeout;
 use wasm_bindgen::JsCast;
 
-use yew::html::IntoPropValue;
+use yew::html::{IntoEventCallback, IntoPropValue};
 use yew::prelude::*;
 use yew::virtual_dom::{Key, VComp, VNode};
 
@@ -212,6 +212,10 @@ pub struct DataTable<S: DataStore> {
 
     #[prop_or_default]
     pub multiselect_mode: MultiSelectMode,
+
+    /// Table scroll callback
+    #[prop_or_default]
+    pub on_table_scroll: Option<Callback<Event>>,
 }
 
 impl<S: DataStore> AsClassesMut for DataTable<S> {
@@ -456,6 +460,12 @@ impl<S: DataStore> DataTable<S> {
         self.set_multiselect_mode(multiselect_mode);
         self
     }
+
+    /// Builder style method to set the table scroll callback.
+    pub fn on_table_scroll(mut self, cb: impl IntoEventCallback<Event>) -> Self {
+        self.on_table_scroll = cb.into_event_callback();
+        self
+    }
 }
 
 #[derive(Default)]
@@ -1602,8 +1612,18 @@ impl<S: DataStore + 'static> Component for PwtDataTable<S> {
         let column_widths =
             self.column_widths.iter().sum::<f64>() + self.scrollbar_size.unwrap_or_default();
 
+        let on_scroll = {
+            let on_scroll = props.on_table_scroll.clone();
+            move |event: Event| {
+                if let Some(on_scroll) = &on_scroll {
+                    on_scroll.emit(event);
+                }
+            }
+        };
+
         let viewport = Container::new()
             .node_ref(self.scroll_ref.clone())
+            .onscroll(on_scroll)
             .key(Key::from("table-viewport"))
             .class("pwt-flex-fill")
             .style(
-- 
2.39.5



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

[yew-devel] [PATCH yew-comp 1/2] tasks: improve behavior of loading additional tasks

[Dominik Csapak]

currently, the task list gets refreshed any of the last 20 entries in
the store get rendered. This also happens though when e.g. the selection
changes or other things cause a redraw even if the position in the list
did not change, which cause unnecessary reloads.

Instead, use the scroll callback of DataTable to only load additional
task when the users scrolls into the last 500px (20 rows would be >600px
for the Desktop theme, and <480px for the crisp theme, so 500px is a
good middle ground).

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/tasks.rs | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/src/tasks.rs b/src/tasks.rs
index 67fa7b1..3c531c1 100644
--- a/src/tasks.rs
+++ b/src/tasks.rs
@@ -10,6 +10,8 @@ use pwt::widget::form::{Field, Form, FormContext, InputType};
 use gloo_timers::callback::Timeout;
 use html::IntoEventCallback;
 use serde_json::Map;
+use wasm_bindgen::JsCast;
+use web_sys::HtmlElement;
 use yew::html::IntoPropValue;
 use yew::virtual_dom::{VComp, VNode};
 
@@ -32,7 +34,7 @@ use super::{TaskStatusSelector, TaskTypeSelector};
 
 const FILTER_UPDATE_BUFFER_MS: u32 = 150;
 const BATCH_LIMIT: u64 = 500;
-const LOAD_BUFFER_ROWS: usize = 20;
+const LOAD_BUFFER_PX: i32 = 500;
 
 #[derive(PartialEq, Properties)]
 #[builder]
@@ -168,12 +170,7 @@ impl LoadableComponent for ProxmoxTasks {
             FormContext::new().on_change(ctx.link().callback(|_| Msg::UpdateFilter));
 
         let row_render_callback = DataTableRowRenderCallback::new({
-            let store = store.clone();
-            let link = link.clone();
             move |args: &mut _| {
-                if args.row_index() > store.data_len().saturating_sub(LOAD_BUFFER_ROWS) {
-                    link.send_message(Msg::LoadBatch(store.data_len() as u64));
-                }
                 let record: &TaskListItem = args.record();
                 match record.status.as_deref() {
                     Some("RUNNING" | "OK") | None => {}
@@ -399,6 +396,22 @@ impl LoadableComponent for ProxmoxTasks {
         DataTable::new(columns, self.store.clone())
             .class("pwt-flex-fit")
             .selection(self.selection.clone())
+            .on_table_scroll({
+                let link = ctx.link().clone();
+                let store = self.store.clone();
+                move |event: Event| {
+                    if let Some(target) = event
+                        .target()
+                        .and_then(|target| target.dyn_into::<HtmlElement>().ok())
+                    {
+                        if target.scroll_height() - (target.scroll_top() + target.client_height())
+                            < LOAD_BUFFER_PX
+                        {
+                            link.send_message(Msg::LoadBatch(store.data_len() as u64));
+                        }
+                    }
+                }
+            })
             .on_row_dblclick(move |_: &mut _| {
                 link.send_message(Msg::ShowTask);
             })
-- 
2.39.5



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

[yew-devel] [PATCH yew-comp 2/2] tasks: don't add entries we already loaded

[Dominik Csapak]

If there is a new task in between loads due to scrolling, we got
old tasks from the api that we already loaded.

This happens because:
* tasks are sorted descending by their start time
* we initially request up to 500 from start 0
* we save the amount of tasks there are
* on a new load due to scrolling, we set start to the amount
  of tasks knew of -> if there are new tasks now, the start offset
  we sent would be too low

To fix this, skip the amount of tasks in the list that were added in the
meantime, which is the new total minus the new total.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
ideally we could implement a kind of cursor for our tasks, so that
this scenario would not happen, but I'm not sure if this is feasible

alternatively/additionally we could implement some key constraints
for datastores, or use a Store with a HashMap instead of a vector
(we'd have to do the ordering manually in this case though, which
might be a big overhead for big task lists..)

 src/tasks.rs | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/src/tasks.rs b/src/tasks.rs
index 3c531c1..ac8d1c5 100644
--- a/src/tasks.rs
+++ b/src/tasks.rs
@@ -95,7 +95,8 @@ pub enum ViewDialog {
 pub enum Msg {
     Redraw,
     ToggleFilter,
-    LoadBatch(u64), // start
+    LoadBatch(u64),                               // start
+    LoadFinished(Option<u64>, Vec<TaskListItem>), // total
     UpdateFilter,
     ShowTask,
 }
@@ -109,6 +110,7 @@ pub struct ProxmoxTasks {
     last_filter: serde_json::Value,
     load_timeout: Option<Timeout>,
     columns: Rc<Vec<DataTableHeader<TaskListItem>>>,
+    total: Option<u64>,
 }
 
 impl ProxmoxTasks {
@@ -192,6 +194,7 @@ impl LoadableComponent for ProxmoxTasks {
             start: 0,
             load_timeout: None,
             columns: Self::columns(ctx),
+            total: None,
         }
     }
 
@@ -206,8 +209,6 @@ impl LoadableComponent for ProxmoxTasks {
             None => format!("/nodes/{nodename}/tasks"),
         };
 
-        let store = self.store.clone();
-
         let form_context = self.filter_form_context.read();
         let mut filter = form_context.get_submit_data();
 
@@ -234,13 +235,11 @@ impl LoadableComponent for ProxmoxTasks {
         filter["start"] = start.into();
         filter["limit"] = BATCH_LIMIT.into();
 
+        let link = ctx.link().clone();
         Box::pin(async move {
-            let mut data: Vec<_> = crate::http_get(&path, Some(filter)).await?;
-            if start == 0 {
-                store.write().set_data(data);
-            } else {
-                store.write().append(&mut data);
-            }
+            let response = crate::http_get_full(&path, Some(filter)).await?;
+            let total = response.attribs.get("total").and_then(|v| v.as_u64());
+            link.send_message(Msg::LoadFinished(total, response.data));
             Ok(())
         })
     }
@@ -279,6 +278,33 @@ impl LoadableComponent for ProxmoxTasks {
                 }));
                 false
             }
+            Msg::LoadFinished(total, mut data) => {
+                let old_total = self.total.take();
+                self.total = total;
+                if self.start == 0 {
+                    self.store.write().set_data(data);
+                } else {
+                    // if new elements were added at the front, we need to skip as many from the
+                    // newly gathered list, otherwise we have duplicate items
+                    match (old_total, self.total) {
+                        (Some(old), Some(new)) if new > old => {
+                            let to_skip = (new - old) as usize;
+
+                            if to_skip <= data.len() {
+                                data.drain(..to_skip);
+                            }
+                        }
+                        _ => {}
+                    };
+
+                    if data.is_empty() {
+                        return false;
+                    } else {
+                        self.store.write().append(&mut data);
+                    }
+                }
+                true
+            }
             Msg::ShowTask => {
                 if let Some(on_show_task) = &ctx.props().on_show_task {
                     let selected_item = self
-- 
2.39.5



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

Re: [yew-devel] [PATCH yew-widget-toolkit 1/2] fix #6382: widget: data table: prevent duplicate yew keys for rows

[Dietmar Maurer]

This is not the solution, because you cannot relate the new
keys to the original ones. So you can no longer test what records have
changed, i.e. when the record order changes.

> To prevent that for the data table, prefix the record key with the index
> into the store, the combination of which must be unique in this list.


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

[yew-devel] applied: [PATCH yew-widget-toolkit 2/2] widget: data table: add table scroll callback

[Dietmar Maurer]

applied


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

Re: [yew-devel] [PATCH yew-comp 2/2] tasks: don't add entries we already loaded

[Dietmar Maurer]

> On 7.5.2025 16:38 CEST Dominik Csapak <d.csapak@proxmox.com> wrote:
> ....
> * on a new load due to scrolling, we set start to the amount
>   of tasks knew of -> if there are new tasks now, the start offset
>   we sent would be too low

Maybe we can use the starttime filter instead?


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