fix: newly-created group rows appear out of order (#6212)

* fix: newly-created group rows appear out of order

* test: fix tests

* fix: dont re-insert nor insert into no-status

* fix: clippy

frontend/appflowy_flutter/macos/Podfile.lock

@@ -142,7 +142,7 @@ SPEC CHECKSUMS:
   HotKey: e96d8a2ddbf4591131e2bb3f54e69554d90cdca6
   hotkey_manager: c32bf0bfe8f934b7bc17ab4ad5c4c142960b023c
   irondash_engine_context: da62996ee25616d2f01bbeb85dc115d813359478
-  local_notifier: c6c371695f914641ab7bc8601944f7e358632d0b
+  local_notifier: e9506bc66fc70311e8bc7291fb70f743c081e4ff
   package_info_plus: fa739dd842b393193c5ca93c26798dff6e3d0e0c
   path_provider_foundation: 3784922295ac71e43754bd15e0653ccfd36a147c
   ReachabilitySwift: 7f151ff156cea1481a8411701195ac6a984f4979

frontend/rust-lib/flowy-database2/src/services/database_view/view_editor.rs

@@ -196,9 +196,12 @@ impl DatabaseViewEditor {
       let mut rows = vec![Arc::new(row.clone())];
       self.v_filter_rows(&mut rows).await;
       if let Some(row) = rows.pop() {
-        let changesets = controller.did_create_row(&row, index);
-        for changeset in changesets {
-          notify_did_update_group_rows(changeset).await;
+        if let Some(changesets) = controller.did_create_row(&row, index).await {
+          for changeset in changesets {
+            if !changeset.is_empty() {
+              notify_did_update_group_rows(changeset).await;
+            }
+          }
         }
       }
     }

frontend/rust-lib/flowy-database2/src/services/group/action.rs

@@ -137,7 +137,11 @@ pub trait GroupController: Send + Sync {
   /// Returns a changeset payload to be sent as a notification.
   ///
   /// * `row_detail`: the newly-created row
-  fn did_create_row(&mut self, row: &Row, index: usize) -> Vec<GroupRowsNotificationPB>;
+  async fn did_create_row(
+    &mut self,
+    row: &Row,
+    index: usize,
+  ) -> Option<Vec<GroupRowsNotificationPB>>;
 
   /// Called after a row's cell data is changed, this moves the row to the
   /// correct group. It may also insert a new group and/or remove an old group.

frontend/rust-lib/flowy-database2/src/services/group/controller.rs

@@ -154,6 +154,57 @@ where
     changeset.deleted_rows.extend(deleted_row_ids);
     Some(changeset)
   }
+
+  fn insert_row_into_group(
+    &mut self,
+    new_row: &Row,
+    index_in_row_order_array: usize,
+    group_id: &str,
+    all_rows: &[Arc<Row>],
+  ) -> Option<usize> {
+    let mut delta: i64 = 0;
+    let mut left_reached = false;
+    let mut right_reached = false;
+
+    let group_data = self.context.get_mut_group(group_id)?;
+
+    // from the index in the row order array, find the nearest row that's also in the same group
+    let index = loop {
+      delta = if delta > 0 { -delta } else { -(delta - 1) };
+
+      let query_index = index_in_row_order_array as i64 + delta;
+
+      if query_index < 0 {
+        if right_reached && left_reached {
+          break None;
+        } else {
+          left_reached = true;
+          continue;
+        }
+      } else if query_index >= all_rows.len() as i64 {
+        if right_reached && left_reached {
+          break None;
+        } else {
+          right_reached = true;
+          continue;
+        }
+      }
+
+      let row = &all_rows[query_index as usize];
+
+      if let Some(index) = group_data.index_of_row(&row.id.clone()) {
+        if delta > 0 {
+          break Some(index);
+        } else {
+          break Some(index + 1);
+        }
+      }
+    }?;
+
+    group_data.insert_row(index, new_row.clone());
+
+    Some(index)
+  }
 }
 
 #[async_trait]
@@ -229,54 +280,71 @@ where
     self.context.move_group(from_group_id, to_group_id)
   }
 
-  fn did_create_row(&mut self, row: &Row, index: usize) -> Vec<GroupRowsNotificationPB> {
-    let mut changesets: Vec<GroupRowsNotificationPB> = vec![];
-
+  async fn did_create_row(
+    &mut self,
+    row: &Row,
+    index: usize,
+  ) -> Option<Vec<GroupRowsNotificationPB>> {
     let cell = match row.cells.get(&self.grouping_field_id) {
-      None => self.placeholder_cell(),
-      Some(cell) => Some(cell.clone()),
+      None => self.placeholder_cell()?,
+      Some(cell) => cell.clone(),
     };
 
-    if let Some(cell) = cell {
-      let cell_data = <T as TypeOption>::CellData::from(&cell);
+    let mut changesets: Vec<GroupRowsNotificationPB> = vec![];
 
-      let mut suitable_group_ids = vec![];
+    let cell_data = <T as TypeOption>::CellData::from(&cell);
 
-      for group in self.get_all_groups() {
-        if self.can_group(&group.id, &cell_data) {
-          suitable_group_ids.push(group.id.clone());
-          let changeset = GroupRowsNotificationPB::insert(
-            group.id.clone(),
-            vec![InsertedRowPB {
-              row_meta: (*row).clone().into(),
-              index: Some(index as i32),
-              is_new: true,
-            }],
-          );
-          changesets.push(changeset);
-        }
-      }
-      if !suitable_group_ids.is_empty() {
-        for group_id in suitable_group_ids.iter() {
-          if let Some(group) = self.context.get_mut_group(group_id) {
-            group.add_row((*row).clone());
+    let mut did_insert_into_status_group = false;
+
+    let all_rows = self.delegate.get_all_rows(&self.context.view_id).await;
+
+    let group_ids = self
+      .context
+      .groups()
+      .into_iter()
+      .filter(|group| group.id != self.grouping_field_id)
+      .map(|group| group.id.clone())
+      .collect::<Vec<_>>();
+
+    for group_id in group_ids {
+      if self.can_group(&group_id, &cell_data) {
+        // make sure that the row isn't already in the group data
+        if let Some((_, group_data)) = self.get_group(&group_id) {
+          if group_data.contains_row(&row.id) {
+            did_insert_into_status_group = true;
+            continue;
           }
         }
-      } else if let Some(no_status_group) = self.context.get_mut_no_status_group() {
-        no_status_group.add_row((*row).clone());
-        let changeset = GroupRowsNotificationPB::insert(
-          no_status_group.id.clone(),
-          vec![InsertedRowPB {
-            row_meta: (*row).clone().into(),
-            index: Some(index as i32),
-            is_new: true,
-          }],
-        );
-        changesets.push(changeset);
+        if let Some(index_in_group) = self.insert_row_into_group(row, index, &group_id, &all_rows) {
+          did_insert_into_status_group = true;
+
+          changesets.push(GroupRowsNotificationPB::insert(
+            group_id.clone(),
+            vec![InsertedRowPB {
+              row_meta: (*row).clone().into(),
+              index: Some(index_in_group as i32),
+              is_new: true,
+            }],
+          ))
+        }
       }
     }
 
-    changesets
+    if !did_insert_into_status_group {
+      let group_id = self.grouping_field_id.clone();
+      if let Some(index_in_group) = self.insert_row_into_group(row, index, &group_id, &all_rows) {
+        changesets.push(GroupRowsNotificationPB::insert(
+          self.grouping_field_id.clone(),
+          vec![InsertedRowPB {
+            row_meta: (*row).clone().into(),
+            index: Some(index_in_group as i32),
+            is_new: true,
+          }],
+        ))
+      }
+    }
+
+    Some(changesets)
   }
 
   fn did_update_group_row(

frontend/rust-lib/flowy-database2/src/services/group/controller_impls/default_controller.rs

@@ -71,17 +71,21 @@ impl GroupController for DefaultGroupController {
     Ok(())
   }
 
-  fn did_create_row(&mut self, row: &Row, index: usize) -> Vec<GroupRowsNotificationPB> {
-    self.group.add_row((*row).clone());
+  async fn did_create_row(
+    &mut self,
+    row: &Row,
+    index: usize,
+  ) -> Option<Vec<GroupRowsNotificationPB>> {
+    self.group.insert_row(index, row.clone());
 
-    vec![GroupRowsNotificationPB::insert(
+    Some(vec![GroupRowsNotificationPB::insert(
       self.group.id.clone(),
       vec![InsertedRowPB {
         row_meta: (*row).clone().into(),
         index: Some(index as i32),
         is_new: true,
       }],
-    )]
+    )])
   }
 
   fn did_update_group_row(

frontend/rust-lib/flowy-database2/src/services/group/entities.rs

@@ -133,15 +133,13 @@ impl GroupData {
 
   pub fn add_row(&mut self, row: Row) {
     match self.rows.iter().find(|r| r.id == row.id) {
-      None => {
-        self.rows.push(row);
-      },
+      None => self.rows.push(row),
       Some(_) => {},
     }
   }
 
   pub fn insert_row(&mut self, index: usize, row: Row) {
-    if index < self.rows.len() {
+    if index <= self.rows.len() {
       self.rows.insert(index, row);
     } else {
       tracing::error!(

frontend/rust-lib/flowy-database2/tests/database/group_test/script.rs

@@ -3,7 +3,9 @@ use collab_database::fields::Field;
 use collab_database::rows::RowId;
 use std::time::Duration;
 
-use flowy_database2::entities::{CreateRowPayloadPB, FieldType, GroupPB, RowMetaPB};
+use flowy_database2::entities::{
+  CreateRowPayloadPB, FieldType, GroupPB, OrderObjectPositionPB, RowMetaPB,
+};
 use flowy_database2::services::cell::{
   delete_select_option_cell, insert_date_cell, insert_select_option_cell, insert_url_cell,
 };
@@ -36,6 +38,7 @@ pub enum GroupScript {
   },
   CreateRow {
     group_index: usize,
+    position: OrderObjectPositionPB,
   },
   DeleteRow {
     group_index: usize,
@@ -131,16 +134,18 @@ impl DatabaseGroupTest {
         row_index,
         row,
       } => {
-        //
         let group = self.group_at_index(group_index).await;
         let compare_row = group.rows.get(row_index).unwrap().clone();
         assert_eq!(row.id, compare_row.id);
       },
-      GroupScript::CreateRow { group_index } => {
+      GroupScript::CreateRow {
+        group_index,
+        position,
+      } => {
         let group = self.group_at_index(group_index).await;
         let params = CreateRowPayloadPB {
           view_id: self.view_id.clone(),
-          row_position: Default::default(),
+          row_position: position,
           group_id: Some(group.group_id),
           data: Default::default(),
         };

frontend/rust-lib/flowy-database2/tests/database/group_test/test.rs

@@ -1,6 +1,8 @@
 use crate::database::group_test::script::DatabaseGroupTest;
 use crate::database::group_test::script::GroupScript::*;
 use collab_database::entity::SelectOption;
+use flowy_database2::entities::OrderObjectPositionPB;
+use flowy_database2::entities::OrderObjectPositionTypePB;
 
 #[tokio::test]
 async fn group_init_test() {
@@ -201,20 +203,79 @@ async fn group_move_row_to_other_group_and_reorder_from_bottom_to_up_test() {
   ];
   test.run_scripts(scripts).await;
 }
+
 #[tokio::test]
 async fn group_create_row_test() {
   let mut test = DatabaseGroupTest::new().await;
+
+  let group1 = test.group_at_index(1).await;
+
   let scripts = vec![
-    CreateRow { group_index: 1 },
+    CreateRow {
+      group_index: 1,
+      position: Default::default(),
+    },
     AssertGroupRowCount {
       group_index: 1,
       row_count: 3,
     },
-    CreateRow { group_index: 2 },
-    CreateRow { group_index: 2 },
+    AssertRow {
+      group_index: 1,
+      row_index: 0,
+      row: group1.rows.first().unwrap().clone(),
+    },
+    AssertRow {
+      group_index: 1,
+      row_index: 1,
+      row: group1.rows.get(1).unwrap().clone(),
+    },
+  ];
+  test.run_scripts(scripts).await;
+
+  let group1 = test.group_at_index(1).await;
+
+  let scripts = vec![
+    CreateRow {
+      group_index: 1,
+      position: OrderObjectPositionPB {
+        position: OrderObjectPositionTypePB::Before,
+        object_id: Some(group1.rows.first().unwrap().clone().id),
+      },
+    },
+    CreateRow {
+      group_index: 1,
+      position: OrderObjectPositionPB {
+        position: OrderObjectPositionTypePB::After,
+        object_id: Some(group1.rows.first().unwrap().clone().id),
+      },
+    },
+    CreateRow {
+      group_index: 1,
+      position: OrderObjectPositionPB {
+        position: OrderObjectPositionTypePB::Before,
+        object_id: Some(group1.rows.get(2).unwrap().clone().id),
+      },
+    },
+    CreateRow {
+      group_index: 1,
+      position: OrderObjectPositionPB {
+        position: OrderObjectPositionTypePB::After,
+        object_id: Some(group1.rows.get(2).unwrap().clone().id),
+      },
+    },
     AssertGroupRowCount {
-      group_index: 2,
-      row_count: 4,
+      group_index: 1,
+      row_count: 7,
+    },
+    AssertRow {
+      group_index: 1,
+      row_index: 1,
+      row: group1.rows.first().unwrap().clone(),
+    },
+    AssertRow {
+      group_index: 1,
+      row_index: 5,
+      row: group1.rows.get(2).unwrap().clone(),
     },
   ];
   test.run_scripts(scripts).await;