fix(tui): sort tasks in state not just view (#9373)

### Description Move the new task ordering from just the view to the underlying state so that the task list syncs up with the pane. ### Testing Instructions Added unit testing for new insertion order.
vercel · Nov 4, 2024 · cb5a8e6 · cb5a8e6
1 parent 73637f8
commit cb5a8e6
Show file tree

Hide file tree

Showing 3 changed files with 155 additions and 34 deletions.
diff --git a/crates/turborepo-ui/src/tui/app.rs b/crates/turborepo-ui/src/tui/app.rs
@@ -326,7 +326,8 @@ impl<W> App<W> {
             .ok_or_else(|| Error::TaskNotFound { name: task.into() })?;
 
         let running = self.tasks_by_status.running.remove(running_idx);
-        self.tasks_by_status.finished.push(running.finish(result));
+        self.tasks_by_status
+            .insert_finished_task(running.finish(result));
 
         self.tasks
             .get_mut(task)

diff --git a/crates/turborepo-ui/src/tui/table.rs b/crates/turborepo-ui/src/tui/table.rs
@@ -1,4 +1,3 @@
-use itertools::Itertools;
 use ratatui::{
     layout::{Constraint, Rect},
     style::{Color, Modifier, Style, Stylize},
@@ -53,39 +52,29 @@ impl<'b> TaskTable<'b> {
     }
 
     fn finished_rows(&self) -> impl Iterator<Item = Row> + '_ {
-        self.tasks_by_type
-            .finished
-            .iter()
-            // note we can't use the default Ord impl because
-            // we want failed tasks first
-            .sorted_by_key(|task| match task.result() {
-                TaskResult::Failure => 0,
-                TaskResult::Success => 1,
-                TaskResult::CacheHit => 2,
-            })
-            .map(move |task| {
-                let name = if matches!(task.result(), TaskResult::CacheHit) {
-                    Cell::new(Text::styled(task.name(), Style::default().italic()))
-                } else {
-                    Cell::new(task.name())
-                };
+        self.tasks_by_type.finished.iter().map(move |task| {
+            let name = if matches!(task.result(), TaskResult::CacheHit) {
+                Cell::new(Text::styled(task.name(), Style::default().italic()))
+            } else {
+                Cell::new(task.name())
+            };
 
-                Row::new(vec![
-                    name,
-                    match task.result() {
-                        // matches Next.js (and many other CLI tools) https://github.com/vercel/next.js/blob/1a04d94aaec943d3cce93487fea3b8c8f8898f31/packages/next/src/build/output/log.ts
-                        TaskResult::Success => {
-                            Cell::new(Text::styled("✓", Style::default().green().bold()))
-                        }
-                        TaskResult::CacheHit => {
-                            Cell::new(Text::styled("⊙", Style::default().magenta()))
-                        }
-                        TaskResult::Failure => {
-                            Cell::new(Text::styled("⨯", Style::default().red().bold()))
-                        }
-                    },
-                ])
-            })
+            Row::new(vec![
+                name,
+                match task.result() {
+                    // matches Next.js (and many other CLI tools) https://github.com/vercel/next.js/blob/1a04d94aaec943d3cce93487fea3b8c8f8898f31/packages/next/src/build/output/log.ts
+                    TaskResult::Success => {
+                        Cell::new(Text::styled("✓", Style::default().green().bold()))
+                    }
+                    TaskResult::CacheHit => {
+                        Cell::new(Text::styled("⊙", Style::default().magenta()))
+                    }
+                    TaskResult::Failure => {
+                        Cell::new(Text::styled("⨯", Style::default().red().bold()))
+                    }
+                },
+            ])
+        })
     }
 
     fn running_rows(&self) -> impl Iterator<Item = Row> + '_ {

diff --git a/crates/turborepo-ui/src/tui/task.rs b/crates/turborepo-ui/src/tui/task.rs
@@ -103,6 +103,7 @@ impl Task<Finished> {
     }
 }
 
+#[derive(Default)]
 pub struct TaskNamesByStatus {
     pub running: Vec<String>,
     pub planned: Vec<String>,
@@ -117,6 +118,10 @@ pub struct TasksByStatus {
 }
 
 impl TasksByStatus {
+    pub fn new() -> Self {
+        Self::default()
+    }
+
     pub fn all_empty(&self) -> bool {
         self.planned.is_empty() && self.finished.is_empty() && self.running.is_empty()
     }
@@ -189,4 +194,130 @@ impl TasksByStatus {
         );
         self.planned.sort_unstable();
     }
+
+    /// Insert a finished task into the correct place in the finished section.
+    /// The order of `finished` is expected to be: failure, success, cached
+    /// with each subsection being sorted by finish time.
+    /// Returns the index task was inserted at
+    pub fn insert_finished_task(&mut self, task: Task<Finished>) -> usize {
+        let index = match task.result() {
+            TaskResult::Failure => self
+                .finished
+                .iter()
+                .enumerate()
+                .skip_while(|(_, task)| task.result() == TaskResult::Failure)
+                .map(|(idx, _)| idx)
+                .next(),
+            TaskResult::Success => self
+                .finished
+                .iter()
+                .enumerate()
+                .skip_while(|(_, task)| {
+                    task.result() == TaskResult::Failure || task.result() == TaskResult::Success
+                })
+                .map(|(idx, _)| idx)
+                .next(),
+            TaskResult::CacheHit => None,
+        }
+        .unwrap_or(self.finished.len());
+        self.finished.insert(index, task);
+        index
+    }
+}
+
+#[cfg(test)]
+mod test {
+    use test_case::test_case;
+
+    use super::*;
+
+    struct TestCase {
+        failed: &'static [&'static str],
+        passed: &'static [&'static str],
+        cached: &'static [&'static str],
+        result: TaskResult,
+        expected_index: usize,
+    }
+
+    impl TestCase {
+        pub const fn new(result: TaskResult, expected_index: usize) -> Self {
+            Self {
+                failed: &[],
+                passed: &[],
+                cached: &[],
+                result,
+                expected_index,
+            }
+        }
+
+        pub const fn failed<const N: usize>(mut self, failed: &'static [&'static str; N]) -> Self {
+            self.failed = failed.as_slice();
+            self
+        }
+
+        pub const fn passed<const N: usize>(mut self, passed: &'static [&'static str; N]) -> Self {
+            self.passed = passed.as_slice();
+            self
+        }
+
+        pub const fn cached<const N: usize>(mut self, cached: &'static [&'static str; N]) -> Self {
+            self.cached = cached.as_slice();
+            self
+        }
+
+        pub fn tasks(&self) -> TasksByStatus {
+            let failed = self.failed.iter().map(|name| {
+                Task::new(name.to_string())
+                    .start()
+                    .finish(TaskResult::Failure)
+            });
+            let passed = self.passed.iter().map(|name| {
+                Task::new(name.to_string())
+                    .start()
+                    .finish(TaskResult::Success)
+            });
+            let cached = self.passed.iter().map(|name| {
+                Task::new(name.to_string())
+                    .start()
+                    .finish(TaskResult::CacheHit)
+            });
+            TasksByStatus {
+                running: Vec::new(),
+                planned: Vec::new(),
+                finished: failed.chain(passed).chain(cached).collect(),
+            }
+        }
+    }
+
+    const EMPTY_FAIL: TestCase = TestCase::new(TaskResult::Failure, 0);
+    const EMPTY_PASS: TestCase = TestCase::new(TaskResult::Success, 0);
+    const EMPTY_CACHE: TestCase = TestCase::new(TaskResult::CacheHit, 0);
+    const BASIC_FAIL: TestCase = TestCase::new(TaskResult::Failure, 1)
+        .failed(&["fail"])
+        .passed(&["passed"])
+        .cached(&["cached"]);
+    const BASIC_PASS: TestCase = TestCase::new(TaskResult::Success, 2)
+        .failed(&["fail"])
+        .passed(&["passed"])
+        .cached(&["cached"]);
+    const BASIC_CACHE: TestCase = TestCase::new(TaskResult::CacheHit, 3)
+        .failed(&["fail"])
+        .passed(&["passed"])
+        .cached(&["cached"]);
+
+    #[test_case(EMPTY_FAIL)]
+    #[test_case(EMPTY_PASS)]
+    #[test_case(EMPTY_CACHE)]
+    #[test_case(BASIC_FAIL)]
+    #[test_case(BASIC_PASS)]
+    #[test_case(BASIC_CACHE)]
+    fn test_finished_task(test_case: TestCase) {
+        let mut tasks = test_case.tasks();
+        let actual = tasks.insert_finished_task(
+            Task::new("inserted".into())
+                .start()
+                .finish(test_case.result),
+        );
+        assert_eq!(actual, test_case.expected_index);
+    }
 }