[yew-devel] [PATCH yew-comp] fix #6382: tasks: don't add duplicate tasks into the list

[yew-devel] [PATCH yew-comp] fix #6382: tasks: don't add duplicate tasks into the list

[Dominik Csapak]

When loading tasks due to reaching the end of the list, the amount of
existing tasks was used as the start parameter for the next load.

New tasks are added at the beginning of the list, so if new tasks were
added in the meantime, the API call would return tasks we already had
in the list. The UPID of the tasks are getting used as keys for the
store to uniquely identify the records. Since those get also used as yew
keys for the vdom by the data table and duplicate keys are not supported
in yew (can lead to a crash) we must avoid this.

To fix this, instead of using the amount of tasks as offset, use the
last task's start time as new 'until' value. With this we know there will
be overlap, but only the tasks started the same second. We can skip them
by finding the cutoff in the newly returned data which is the index of
the UPID of the last one of the current store content.

This makes the 'start' parameter of the LoadBatch update unnecessary,
and we replace it with a 'fresh load' flag that get's reset after every
load, and only gets used by the refesh button and when filters change.
(like we used 'start == 0' before)

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

diff --git a/src/tasks.rs b/src/tasks.rs
index 67fa7b1..3bde629 100644
--- a/src/tasks.rs
+++ b/src/tasks.rs
@@ -93,7 +93,8 @@ pub enum ViewDialog {
 pub enum Msg {
     Redraw,
     ToggleFilter,
-    LoadBatch(u64), // start
+    LoadBatch(bool), // fresh load
+    LoadFinished,
     UpdateFilter,
     ShowTask,
 }
@@ -103,7 +104,7 @@ pub struct ProxmoxTasks {
     show_filter: PersistentState<bool>,
     filter_form_context: FormContext,
     row_render_callback: DataTableRowRenderCallback<TaskListItem>,
-    start: u64,
+    fresh_load: bool,
     last_filter: serde_json::Value,
     load_timeout: Option<Timeout>,
     columns: Rc<Vec<DataTableHeader<TaskListItem>>>,
@@ -172,7 +173,7 @@ impl LoadableComponent for ProxmoxTasks {
             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));
+                    link.send_message(Msg::LoadBatch(false));
                 }
                 let record: &TaskListItem = args.record();
                 match record.status.as_deref() {
@@ -192,7 +193,7 @@ impl LoadableComponent for ProxmoxTasks {
             filter_form_context,
             row_render_callback,
             last_filter: serde_json::Value::Object(Map::new()),
-            start: 0,
+            fresh_load: true,
             load_timeout: None,
             columns: Self::columns(ctx),
         }
@@ -233,17 +234,43 @@ impl LoadableComponent for ProxmoxTasks {
             filter["until"] = until.into();
         }
 
-        let start = self.start;
-        filter["start"] = start.into();
+        let start = match (self.fresh_load, self.store.read().last()) {
+            (false, Some(last)) => last.starttime,
+            _ => 0,
+        };
+        if start > 0 && filter["until"].as_i64().unwrap_or(i64::MAX) > start {
+            filter["until"] = 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);
+                // since we used the starttime from the last existing element,
+                // we know there should be an overlap, so find it
+                let mut store = store.write();
+                if let Some(last) = store.last() {
+                    let mut cutoff = None;
+                    for (idx, new) in data.iter().enumerate() {
+                        if new.upid == last.upid {
+                            cutoff = Some(idx);
+                            break;
+                        }
+                        if new.starttime < last.starttime {
+                            // we did not find the last element, append all
+                            break;
+                        }
+                    }
+                    if let Some(cutoff) = cutoff {
+                        data.drain(0..=cutoff);
+                    }
+                }
+                store.append(&mut data);
             }
+            link.send_message(Msg::LoadFinished);
             Ok(())
         })
     }
@@ -266,7 +293,7 @@ impl LoadableComponent for ProxmoxTasks {
                 }
 
                 self.last_filter = filter_params;
-                self.start = 0;
+                self.fresh_load = true;
 
                 let link = ctx.link().clone();
                 self.load_timeout = Some(Timeout::new(FILTER_UPDATE_BUFFER_MS, move || {
@@ -275,13 +302,17 @@ impl LoadableComponent for ProxmoxTasks {
                 true
             }
             Msg::LoadBatch(start) => {
-                self.start = start;
+                self.fresh_load = start;
                 let link = ctx.link().clone();
                 self.load_timeout = Some(Timeout::new(FILTER_UPDATE_BUFFER_MS, move || {
                     link.send_reload();
                 }));
                 false
             }
+            Msg::LoadFinished => {
+                self.fresh_load = false;
+                false
+            }
             Msg::ShowTask => {
                 if let Some(on_show_task) = &ctx.props().on_show_task {
                     let selected_item = self
@@ -339,7 +370,7 @@ impl LoadableComponent for ProxmoxTasks {
             .with_child({
                 let loading = ctx.loading();
                 let link = ctx.link();
-                Button::refresh(loading).onclick(move |_| link.send_message(Msg::LoadBatch(0)))
+                Button::refresh(loading).onclick(move |_| link.send_message(Msg::LoadBatch(true)))
             });
 
         let filter_classes = classes!(
-- 
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-comp] fix #6382: tasks: don't add duplicate tasks into the list

[Dietmar Maurer]

>  
> -        let start = self.start;
> -        filter["start"] = start.into();
> +        let start = match (self.fresh_load, self.store.read().last()) {
> +            (false, Some(last)) => last.starttime,
> +            _ => 0,
> +        };
> +        if start > 0 && filter["until"].as_i64().unwrap_or(i64::MAX) > start {
> +            filter["until"] = start.into();
> +        }

The naming is quite confusing - maybe rename "start" into "until"?
Also, value "0" is not used (just a marker), so maybe it is cleaner to use an option?


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