diff --git a/crates/dashboard-app/src/captures.rs b/crates/dashboard-app/src/captures.rs index 27fee91..7557d2f 100644 --- a/crates/dashboard-app/src/captures.rs +++ b/crates/dashboard-app/src/captures.rs @@ -1673,3 +1673,216 @@ fn captures_projects_selection_after_reorder() { save(&mut cx, handle, &out, "selection-projects-4-enter"); assert_eq!(editing(&mut cx).as_deref(), Some("gh-x")); } + +/// Issue 91, Work items past the first page: a rank save that keeps the item on the second page keeps it selected. +/// From a pmd that lists in its own order, with 62 items; Load more, then the keyboard from row 55. Each state is saved, +/// then checked, so the old code still writes the state it gets wrong. +#[test] +#[ignore = "writes issue 91 deep-page Work items selection states offscreen"] +fn captures_work_deep_page_selection() { + use crate::work_items_tests::{FakeWork, work_item}; + use std::sync::atomic::Ordering; + let out = PathBuf::from(std::env::var_os("CAPTURE_DIR").expect("CAPTURE_DIR")); + std::fs::create_dir_all(&out).unwrap(); + let mut cx = headless(); + let projects = [ + "gh-x", + "codexbar", + "hs-buddy", + "chief", + "set-it-free-loop", + "project-dashboard", + ]; + let items = (0..62) + .map(|ix| { + let project = projects[ix % projects.len()]; + work_item(ix, project, "issue", &format!("Maintenance item {}", ix + 1)) + }) + .collect(); + let backend = FakeWork::new(&["read", "write:work"], items); + backend.in_pmd_order.store(true, Ordering::SeqCst); + let (handle, manage) = open_manage_dialog( + &mut cx, + backend.clone(), + None, + dashboard_core::write::Feature::WorkItems, + ); + let listed = |cx: &mut HeadlessAppContext| { + cx.update(|cx| { + let work = manage.read(cx).work_items().unwrap(); + let numbers: Vec = work.items().iter().map(|item| item.number).collect(); + (numbers, work.selected()) + }) + }; + let editing = |cx: &mut HeadlessAppContext| { + cx.update(|cx| { + manage + .read(cx) + .work_items() + .unwrap() + .editor_item() + .map(|item| item.number) + }) + }; + let click = |cx: &mut HeadlessAppContext, id: &str| { + let id = gpui_kit::SharedString::from(id.to_owned()); + cx.update_window(handle, |_, window, cx| window.click(id, cx)).unwrap(); + settle(cx); + }; + + // Load more brings the second page. Open the first row and go Back by keyboard, which focuses the list, then Down + // to row 55, #255. + click(&mut cx, "work-load-more"); + click(&mut cx, "work-row-0"); + tab_to(&mut cx, handle, "tab", "work-back"); + key(&mut cx, handle, &["enter"]); + for _ in 0..55 { + key(&mut cx, handle, &["down"]); + } + save(&mut cx, handle, &out, "deep-work-1-before"); + let (numbers, selected) = listed(&mut cx); + assert_eq!((numbers.len(), numbers[55], selected), (62, 255, 55)); + + // Enter opens it; rank 5 (typed into the rank field), Ctrl+Enter saves it. The reload reads both pages to find it. + key(&mut cx, handle, &["enter"]); + assert_eq!(editing(&mut cx), Some(255)); + let rank = cx.update(|cx| manage.read(cx).input("rank").unwrap()); + cx.update_window(handle, |_, window, cx| { + rank.update(cx, |input, cx| input.set_value("5", window, cx)) + }) + .unwrap(); + let before = backend.lists.lock().unwrap().len(); + key(&mut cx, handle, &["ctrl-enter"]); + settle(&mut cx); + save(&mut cx, handle, &out, "deep-work-2-saved"); + let writes = backend.writes.lock().unwrap().clone(); + assert_eq!(writes.len(), 1); + assert_eq!(writes[0].path, "v1/work/01JBXWORK0055"); + assert_eq!(writes[0].body, serde_json::json!({"rank": 5})); + + // Tab to Back to list, Enter: the highlight is on #255, now last, on the second page, scrolled into view. + tab_to(&mut cx, handle, "tab", "work-back"); + key(&mut cx, handle, &["enter"]); + save(&mut cx, handle, &out, "deep-work-3-after-back"); + assert!(has_focus(&mut cx, handle, "work-list")); + let (numbers, selected) = listed(&mut cx); + assert_eq!((numbers.len(), numbers[61], selected), (62, 255, 61)); + let cursors: Vec> = backend.lists.lock().unwrap()[before..] + .iter() + .map(|query| query.iter().find(|(k, _)| k == "cursor").map(|(_, v)| v.clone())) + .collect(); + assert_eq!(cursors, [None, Some("50".to_owned())], "the reload read both pages"); + + // Enter reopens #255. + key(&mut cx, handle, &["enter"]); + save(&mut cx, handle, &out, "deep-work-4-enter"); + assert_eq!(editing(&mut cx), Some(255)); +} + +/// Issue 91, a reload that stops before pmd's list ends: a rank save on #255, then a listing whose second page hands +/// back its own cursor. The rows from before the save stay with #255 selected, beside the error and Retry. Keyboard +/// from row 55; each state is saved, then checked. +#[test] +#[ignore = "writes issue 91 stopped-reload Work items state offscreen"] +fn captures_work_reload_stopped() { + use crate::work_items_tests::{FakeWork, work_item}; + use std::sync::atomic::Ordering; + let out = PathBuf::from(std::env::var_os("CAPTURE_DIR").expect("CAPTURE_DIR")); + std::fs::create_dir_all(&out).unwrap(); + let mut cx = headless(); + let projects = [ + "gh-x", + "codexbar", + "hs-buddy", + "chief", + "set-it-free-loop", + "project-dashboard", + ]; + let items = (0..130) + .map(|ix| { + let project = projects[ix % projects.len()]; + work_item(ix, project, "issue", &format!("Maintenance item {}", ix + 1)) + }) + .collect(); + let backend = FakeWork::new(&["read", "write:work"], items); + backend.in_pmd_order.store(true, Ordering::SeqCst); + let (handle, manage) = open_manage_dialog( + &mut cx, + backend.clone(), + None, + dashboard_core::write::Feature::WorkItems, + ); + let listed = |cx: &mut HeadlessAppContext| { + cx.update(|cx| { + let work = manage.read(cx).work_items().unwrap(); + let numbers: Vec = work.items().iter().map(|item| item.number).collect(); + (numbers, work.selected()) + }) + }; + let editing = |cx: &mut HeadlessAppContext| { + cx.update(|cx| { + manage + .read(cx) + .work_items() + .unwrap() + .editor_item() + .map(|item| item.number) + }) + }; + let click = |cx: &mut HeadlessAppContext, id: &str| { + let id = gpui_kit::SharedString::from(id.to_owned()); + cx.update_window(handle, |_, window, cx| window.click(id, cx)).unwrap(); + settle(cx); + }; + + // All three pages, then Back from the first row's editor focuses the list, and Down to row 55, #255. + click(&mut cx, "work-load-more"); + click(&mut cx, "work-load-more"); + click(&mut cx, "work-row-0"); + tab_to(&mut cx, handle, "tab", "work-back"); + key(&mut cx, handle, &["enter"]); + for _ in 0..55 { + key(&mut cx, handle, &["down"]); + } + let (numbers, selected) = listed(&mut cx); + assert_eq!((numbers.len(), numbers[55], selected), (130, 255, 55)); + + // Enter, rank 5, Ctrl+Enter: the reload's second page hands back its own cursor, so it stops there. + key(&mut cx, handle, &["enter"]); + assert_eq!(editing(&mut cx), Some(255)); + let rank = cx.update(|cx| manage.read(cx).input("rank").unwrap()); + cx.update_window(handle, |_, window, cx| { + rank.update(cx, |input, cx| input.set_value("5", window, cx)) + }) + .unwrap(); + backend.stuck_cursor.store(true, Ordering::SeqCst); + let before = backend.lists.lock().unwrap().len(); + key(&mut cx, handle, &["ctrl-enter"]); + settle(&mut cx); + + // Tab to Back to list, Enter: the rows from before the save, #255 still selected at row 56, the error and Retry. + tab_to(&mut cx, handle, "tab", "work-back"); + key(&mut cx, handle, &["enter"]); + save(&mut cx, handle, &out, "stopped-work-1-after-back"); + let (numbers, selected) = listed(&mut cx); + assert_eq!((numbers.len(), numbers[55], selected), (130, 255, 55)); + let message = cx + .update_window(handle, |_, window, _| { + window.find("dialog-message").label().unwrap_or_default().to_owned() + }) + .unwrap(); + assert!(message.starts_with("The reload stopped after 2 pages"), "{message}"); + let writes = backend.writes.lock().unwrap().clone(); + assert_eq!(writes.len(), 1); + assert_eq!(writes[0].path, "v1/work/01JBXWORK0055"); + let cursors: Vec> = backend.lists.lock().unwrap()[before..] + .iter() + .map(|query| query.iter().find(|(k, _)| k == "cursor").map(|(_, v)| v.clone())) + .collect(); + assert_eq!(cursors, [None, Some("50".to_owned())]); + + // Enter reopens #255. + key(&mut cx, handle, &["enter"]); + save(&mut cx, handle, &out, "stopped-work-2-enter"); + assert_eq!(editing(&mut cx), Some(255)); +} diff --git a/crates/dashboard-app/src/manage/work_items.rs b/crates/dashboard-app/src/manage/work_items.rs index 7fd279e..7cb0f5c 100644 --- a/crates/dashboard-app/src/manage/work_items.rs +++ b/crates/dashboard-app/src/manage/work_items.rs @@ -10,7 +10,7 @@ use std::sync::Arc; use dashboard_core::edit::{Form, Next}; -use dashboard_core::selection; +use dashboard_core::selection::{self, Reload}; use dashboard_core::work::{ self, AUTO, FIELDS, Kind, MAX_REASON, STATUSES, WorkFilter, WorkItem, WorkPage, status_label, }; @@ -68,12 +68,21 @@ pub struct WorkItemsDialog { /// Bumped by every load from the first page, so an older page landing late can't replace a newer list. generation: u64, selected: usize, + /// A reload's pages so far, while it reads on to find the selected item (#91). The rows on screen, and their + /// selection, stay until it is found or the listing ends. + staged: Option, load_more: Control, retry: Control, editor: Option, editors_opened: u64, } +/// The pages a reload has read so far, and how many. +struct Staged { + rows: Vec, + pages: usize, +} + pub(super) struct Editor { /// Unique per opening, so input changes and saves for an editor that has since closed are dropped. key: u64, @@ -227,6 +236,7 @@ impl Manage { load_error: None, generation: 0, selected: 0, + staged: None, load_more: control(cx), retry: control(cx), editor: None, @@ -249,8 +259,9 @@ impl Manage { }; let cursor = if reset { work.generation += 1; - // The old listing's cursor means nothing for the new query. + // The old listing's cursor, and any reload's pages, mean nothing for the new query. work.next_cursor = None; + work.staged = None; None } else { match &work.next_cursor { @@ -277,28 +288,64 @@ impl Manage { return; } work.loading = false; + let mut read_on = false; match result { - Ok(page) => { - if cursor.is_none() { - // Under the same filters the selected item stays selected wherever the new page puts - // it (#91), after a save reorders it or a change elsewhere moves it. Under new filters - // the position stays, as before; a filter chip has already reset it to the first row. - let previous = (work.rows_filter == filter) - .then(|| work.items.get(work.selected).map(|item| item.id.clone())) - .flatten(); - work.rows_filter = filter; - work.items = page.items; - work.selected = selection::reselect( - previous.as_deref(), - work.selected, - work.items.iter().map(|item| item.id.as_str()), - ); - } else { - work.items.extend(page.items); + // A first page, or the next page of a reload still looking for the selected item. + Ok(page) if cursor.is_none() || work.staged.is_some() => { + let (mut rows, pages) = match work.staged.take() { + Some(staged) => (staged.rows, staged.pages + 1), + None => (Vec::new(), 1), + }; + let beyond = page.beyond(cursor.as_deref(), pages); + rows.extend(page.items); + // Under the same filters the selected item stays selected wherever the reload puts it + // (#91), after a save reorders it or a change elsewhere moves it, on whichever page. Under + // new filters the position stays, as before; a filter chip has already reset it to the + // first row. + let previous = (work.rows_filter == filter) + .then(|| work.items.get(work.selected).map(|item| item.id.clone())) + .flatten(); + match selection::reload( + previous.as_deref(), + work.selected, + rows.iter().map(|item| item.id.as_str()), + beyond, + ) { + Reload::More => { + work.staged = Some(Staged { rows, pages }); + work.next_cursor = page.next_cursor; + read_on = true; + } + Reload::Incomplete => { + // Stopped before the listing ended, without the selected item: it may only be unread. + // As with a failed page, the rows on screen and their selection stay, as the last + // good data, beside the error and Retry; the pages read are dropped, not shown as the + // list. + work.next_cursor = None; + work.load_error = Some(format!( + "The reload stopped after {pages} {}, before pmd's list ended and before it \ + reached the selected item. These rows are from before the reload.", + if pages == 1 { "page" } else { "pages" } + )); + } + Reload::Show(selected) => { + work.rows_filter = filter; + work.items = rows; + work.selected = selected; + work.next_cursor = page.next_cursor; + } } + } + Ok(page) => { + work.items.extend(page.items); work.next_cursor = page.next_cursor; } Err(error) => { + if work.staged.take().is_some() { + // A reload that failed part way keeps the rows on screen, with their selection, as the + // last good data. Its cursor belongs to the new listing, so it can't load more of them. + work.next_cursor = None; + } if cursor.is_none() && work.rows_filter != filter { // The rows on screen belong to other filters: drop them rather than mix listings. Under // the same filters they are the last good data and stay, beside the error. @@ -308,6 +355,9 @@ impl Manage { work.load_error = Some(error.to_string()); } } + if read_on { + manage.load_work(id, false, cx); + } cx.notify(); }); }) diff --git a/crates/dashboard-app/src/work_items_tests.rs b/crates/dashboard-app/src/work_items_tests.rs index 1c958e9..f362288 100644 --- a/crates/dashboard-app/src/work_items_tests.rs +++ b/crates/dashboard-app/src/work_items_tests.rs @@ -5,7 +5,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, Mutex}; use dashboard_core::cache::StatusCache; -use dashboard_core::work::WorkItem; +use dashboard_core::work::{self, WorkItem}; use dashboard_core::write::{Precondition, Whoami, WriteBackend, WriteError, WriteRequest}; use gpui_kit::test::TestWindowExt as _; use gpui_kit::{AnyWindowHandle, AppContext as _, Entity, TestAppContext}; @@ -62,6 +62,12 @@ pub(crate) struct FakeWork { /// Lists in pmd's order (effective priority, rank, then newest) instead of the order the items were given in, so /// a priority or rank save moves the item. pub in_pmd_order: AtomicBool, + /// The next listing of a later page (one with a cursor) fails with this. + pub fail_next_page: Mutex>, + /// Later pages answer with the cursor they were asked for, so the listing never moves on. + pub stuck_cursor: AtomicBool, + /// Later pages come back empty but say the listing goes on. + pub empty_pages: AtomicBool, } impl FakeWork { @@ -74,6 +80,9 @@ impl FakeWork { fail_next: Mutex::default(), fail_lists: Mutex::default(), in_pmd_order: AtomicBool::new(false), + fail_next_page: Mutex::default(), + stuck_cursor: AtomicBool::new(false), + empty_pages: AtomicBool::new(false), }) } @@ -111,6 +120,10 @@ impl WriteBackend for FakeWork { if let Some(error) = self.fail_lists.lock().unwrap().clone() { return Err(error); } + let paged = query.iter().any(|(key, _)| key == "cursor"); + if paged && let Some(error) = self.fail_next_page.lock().unwrap().take() { + return Err(error); + } let get = |key: &str| query.iter().find(|(k, _)| k == key).map(|(_, v)| v.clone()); let items = self.items.lock().unwrap(); let mut matching: Vec = items @@ -135,9 +148,17 @@ impl WriteBackend for FakeWork { } let limit: usize = get("limit").unwrap().parse().unwrap(); let start: usize = get("cursor").map_or(0, |cursor| cursor.parse().unwrap()); - let page: Vec = matching.iter().skip(start).take(limit).cloned().collect(); + let page: Vec = if paged && self.empty_pages.load(Ordering::SeqCst) { + Vec::new() + } else { + matching.iter().skip(start).take(limit).cloned().collect() + }; let next = if start + limit < matching.len() { - (start + limit).to_string() + if paged && self.stuck_cursor.load(Ordering::SeqCst) { + start.to_string() + } else { + (start + limit).to_string() + } } else { String::new() }; @@ -1040,3 +1061,364 @@ async fn a_filter_change_selects_the_first_row_even_when_the_selected_item_is_st assert_eq!(selected(cx, &manage), 0); assert!(backend.writes.lock().unwrap().is_empty()); } + +/// `count` open gh-x items, #200 on, all P2, with `rank` setting each one's rank. +fn ranked(count: usize, rank: impl Fn(usize) -> i64) -> Vec { + (0..count) + .map(|ix| { + let mut item = work_item(ix, "gh-x", "issue", "An item"); + item["rank"] = json!(rank(ix)); + item + }) + .collect() +} + +/// Loads every page, then the keyboard opens row `row`: Back from an editor focuses the list, then Down reaches it. +async fn open_deep_row( + cx: &mut TestAppContext, + handle: AnyWindowHandle, + view: &Entity, + manage: &Entity, + row: usize, +) { + while cx.update(|cx| manage.read(cx).work_items().unwrap().has_more()) { + click(cx, handle, "work-load-more"); + settle(cx, handle, view).await; + } + click(cx, handle, "work-row-0"); + click(cx, handle, "work-back"); + assert!(focused(cx, handle, "work-list")); + for _ in 0..row { + press(cx, handle, "down"); + } + press(cx, handle, "enter"); +} + +fn lists(backend: &FakeWork) -> usize { + backend.lists.lock().unwrap().len() +} + +/// The cursors of the listings made since `from`: `None` for a first page. +fn cursors_since(backend: &FakeWork, from: usize) -> Vec> { + backend.lists.lock().unwrap()[from..] + .iter() + .map(|query| query.iter().find(|(k, _)| k == "cursor").map(|(_, v)| v.clone())) + .collect() +} + +#[gpui_kit::test] +async fn a_save_that_keeps_the_item_past_the_first_page_keeps_it_selected(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(62, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + assert_eq!(editing(cx, &manage), Some(255)); + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + press(cx, handle, "ctrl-enter"); + settle(cx, handle, &view).await; + only_write_is_to(&backend, 255); + click(cx, handle, "work-back"); + assert!(focused(cx, handle, "work-list")); + press(cx, handle, "enter"); + assert_eq!(editing(cx, &manage), Some(255), "Enter reopens the item just saved"); + click(cx, handle, "work-back"); + assert_eq!(selected(cx, &manage), 61, "the highlight stayed on #255"); + assert_eq!(numbers(cx, &manage)[61], 255, "rank 5 sorts #255 last"); + assert_eq!(items(cx, &manage).len(), 62); + assert_eq!( + cursors_since(&backend, before), + [None, Some("50".to_owned())], + "the reload reads on to the page #255 is on now, and no further" + ); +} + +#[gpui_kit::test] +async fn a_save_that_moves_the_item_from_the_third_page_to_the_second_keeps_it_selected(cx: &mut TestAppContext) { + // Ranks 0, 10 and 20 fill the three pages; rank 5 puts the item first on the second page. + let backend = in_pmd_order(ranked(130, |ix| (ix / 50) as i64 * 10)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 110).await; + assert_eq!(editing(cx, &manage), Some(310)); + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + only_write_is_to(&backend, 310); + click(cx, handle, "work-back"); + press(cx, handle, "enter"); + assert_eq!(editing(cx, &manage), Some(310)); + assert_eq!(selected(cx, &manage), 50); + assert_eq!(numbers(cx, &manage)[50], 310); + assert_eq!(items(cx, &manage).len(), 100, "two pages, up to the one with #310"); + assert_eq!(cursors_since(&backend, before), [None, Some("50".to_owned())]); +} + +#[gpui_kit::test] +async fn a_deep_item_that_leaves_the_status_filter_hands_the_selection_to_the_row_in_its_place( + cx: &mut TestAppContext, +) { + let backend = in_pmd_order(ranked(62, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + click(cx, handle, "work-filter-status-ready"); + settle(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + assert_eq!(editing(cx, &manage), Some(255)); + click(cx, handle, "work-status-blocked"); + set_input(cx, handle, &manage, "statusReason", "waiting on review"); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + only_write_is_to(&backend, 255); + click(cx, handle, "work-back"); + press(cx, handle, "enter"); + assert_eq!(editing(cx, &manage), Some(256), "the row in #255's old place"); + assert_eq!(selected(cx, &manage), 55, "the same position, on a listed row"); + assert_eq!(items(cx, &manage).len(), 61); + assert!(!numbers(cx, &manage).contains(&255)); + assert_eq!( + cursors_since(&backend, before), + [None, Some("50".to_owned())], + "read to the end: only then is #255 known to be gone" + ); +} + +#[gpui_kit::test] +async fn a_later_page_that_fails_keeps_the_rows_and_retry_finds_the_item(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(62, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + set_input(cx, handle, &manage, "rank", "5"); + *backend.fail_next_page.lock().unwrap() = Some(WriteError::Network); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + only_write_is_to(&backend, 255); + click(cx, handle, "work-back"); + // The rows on screen stay, as the last good data, with #255 still selected and nothing more to load from them. + assert_eq!(items(cx, &manage).len(), 62); + assert_eq!(selected(cx, &manage), 55); + assert_eq!(numbers(cx, &manage)[55], 255); + assert!(exists(cx, handle, "work-retry")); + assert!(!exists(cx, handle, "work-load-more")); + click(cx, handle, "work-retry"); + settle(cx, handle, &view).await; + assert_eq!(items(cx, &manage).len(), 62); + assert!(!exists(cx, handle, "work-retry")); + assert_eq!(selected(cx, &manage), 61); + assert_eq!( + numbers(cx, &manage)[61], + 255, + "Retry's reload found #255 on the second page" + ); + assert_eq!( + backend.writes.lock().unwrap().len(), + 1, + "a failed read never repeats the write" + ); +} + +/// Loads the second page only, then the keyboard opens row 55, #255: Back from an editor focuses the list. +async fn open_row_55_of_two_pages( + cx: &mut TestAppContext, + handle: AnyWindowHandle, + view: &Entity, + manage: &Entity, +) { + click(cx, handle, "work-load-more"); + settle(cx, handle, view).await; + click(cx, handle, "work-row-0"); + click(cx, handle, "work-back"); + for _ in 0..55 { + press(cx, handle, "down"); + } + press(cx, handle, "enter"); + assert_eq!(editing(cx, manage), Some(255)); +} + +/// After a reload that stopped before pmd's list ended without reaching #255: Back and Enter still reach #255, the +/// `rows` from before the save stay with #255 selected where it was, the error shows beside Retry, and the save wrote +/// once. +fn assert_reload_kept_255( + cx: &mut TestAppContext, + handle: AnyWindowHandle, + manage: &Entity, + backend: &FakeWork, + rows: usize, +) { + click(cx, handle, "work-back"); + press(cx, handle, "enter"); + assert_eq!( + editing(cx, manage), + Some(255), + "Enter still reopens the item just saved" + ); + click(cx, handle, "work-back"); + assert_eq!(selected(cx, manage), 55, "the selection stayed on #255"); + assert_eq!(items(cx, manage).len(), rows, "the rows from before the save stay"); + assert_eq!(numbers(cx, manage)[55], 255); + let message = text_of(cx, handle, "dialog-message"); + assert!( + message.starts_with("The reload stopped after") && message.contains("before pmd's list ended"), + "{message}" + ); + assert!(exists(cx, handle, "work-retry")); + assert!(!exists(cx, handle, "work-load-more")); + only_write_is_to(backend, 255); +} + +#[gpui_kit::test] +async fn a_listing_that_does_not_move_on_stops_the_reload_and_keeps_the_rows(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(130, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + set_input(cx, handle, &manage, "rank", "5"); + backend.stuck_cursor.store(true, Ordering::SeqCst); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + assert_reload_kept_255(cx, handle, &manage, &backend, 130); + assert_eq!( + cursors_since(&backend, before), + [None, Some("50".to_owned())], + "a page that hands back its own cursor stops the reload" + ); +} + +#[gpui_kit::test] +async fn an_empty_page_that_says_the_list_goes_on_stops_the_reload_and_keeps_the_rows(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(130, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + set_input(cx, handle, &manage, "rank", "5"); + backend.empty_pages.store(true, Ordering::SeqCst); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + assert_reload_kept_255(cx, handle, &manage, &backend, 130); + assert_eq!(cursors_since(&backend, before), [None, Some("50".to_owned())]); +} + +#[gpui_kit::test] +async fn a_reload_at_its_page_limit_without_the_item_stops_and_keeps_the_rows(cx: &mut TestAppContext) { + // Rank 5 sorts #255 last of 1,060, on page 22, past the 20 pages a reload reads. + let backend = in_pmd_order(ranked(1060, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_row_55_of_two_pages(cx, handle, &view, &manage).await; + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + assert_reload_kept_255(cx, handle, &manage, &backend, 100); + let read = cursors_since(&backend, before); + assert_eq!(read.len(), work::FOLLOW_PAGES, "exactly the page limit, and no more"); + assert_eq!(read.last(), Some(&Some("950".to_owned()))); +} + +#[gpui_kit::test] +async fn an_item_found_on_the_last_page_a_reload_reads_stays_selected(cx: &mut TestAppContext) { + // Ranks 0 and 10 around rank 5 put #255 at row 990, on page 20, the last a reload reads. + let backend = in_pmd_order(ranked(1060, |ix| if ix < 990 { 0 } else { 10 })); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_row_55_of_two_pages(cx, handle, &view, &manage).await; + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + click(cx, handle, "work-save"); + settle(cx, handle, &view).await; + only_write_is_to(&backend, 255); + click(cx, handle, "work-back"); + press(cx, handle, "enter"); + assert_eq!(editing(cx, &manage), Some(255)); + click(cx, handle, "work-back"); + assert_eq!(selected(cx, &manage), 989); + assert_eq!(items(cx, &manage).len(), 1000, "the 20 pages read, shown as the list"); + assert!(!exists(cx, handle, "dialog-message"), "no error"); + assert!( + cx.update(|cx| manage.read(cx).work_items().unwrap().has_more()), + "Load more goes on from page 21" + ); + assert_eq!(cursors_since(&backend, before).len(), work::FOLLOW_PAGES); +} + +/// Runs one task at a time until `done`, so a test can act between a request and its answer. +fn step_until(cx: &mut TestAppContext, mut done: impl FnMut() -> bool) { + for _ in 0..10_000 { + if done() { + return; + } + if !cx.executor().tick() { + break; + } + } + assert!(done(), "the awaited request never came"); +} + +/// Clicks or presses without running the tasks that starts. +fn act_only(cx: &mut TestAppContext, handle: AnyWindowHandle, click: Option<&str>, key: Option<&str>) { + let click = click.map(|id| gpui_kit::SharedString::from(id.to_owned())); + cx.update_window(handle, |_, window, cx| { + if let Some(id) = click { + window.click(id, cx); + } + if let Some(key) = key { + window.press(key, cx); + } + window.render_frame(cx); + }) + .unwrap(); +} + +#[gpui_kit::test] +async fn a_filter_change_while_a_reload_reads_on_drops_its_late_page(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(62, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + act_only(cx, handle, Some("work-save"), None); + // Stop once the reload has asked for its second page, before that page lands. + step_until(cx, || lists(&backend) == before + 2); + act_only(cx, handle, Some("work-back"), None); + act_only(cx, handle, Some("work-filter-kind-issue"), None); + settle(cx, handle, &view).await; + frame(cx, handle); + only_write_is_to(&backend, 255); + assert_eq!( + cursors_since(&backend, before), + [None, Some("50".to_owned()), None], + "the new filters list from their first page and the old reload reads no further" + ); + assert!(has(&last_query(&backend), "kind", "issue")); + assert_eq!( + items(cx, &manage).len(), + 50, + "the new listing's first page, not the late one mixed in" + ); + assert_eq!(selected(cx, &manage), 0, "new filters start at the first row"); + assert!(cx.update(|cx| manage.read(cx).work_items().unwrap().has_more())); +} + +#[gpui_kit::test] +async fn closing_the_dialog_while_a_reload_reads_on_drops_its_late_page(cx: &mut TestAppContext) { + let backend = in_pmd_order(ranked(62, |_| 0)); + let (handle, view, manage) = open_with(cx, backend.clone()); + open_dialog(cx, handle, &view).await; + open_deep_row(cx, handle, &view, &manage, 55).await; + set_input(cx, handle, &manage, "rank", "5"); + let before = lists(&backend); + act_only(cx, handle, Some("work-save"), None); + step_until(cx, || lists(&backend) == before + 2); + act_only(cx, handle, None, Some("escape")); + settle(cx, handle, &view).await; + frame(cx, handle); + assert_eq!(cx.update(|cx| manage.read(cx).dialog()), None); + only_write_is_to(&backend, 255); + assert_eq!(lists(&backend), before + 2, "nothing more is read for a closed dialog"); +} diff --git a/crates/dashboard-core/src/selection.rs b/crates/dashboard-core/src/selection.rs index 64a33e1..0a24c35 100644 --- a/crates/dashboard-core/src/selection.rs +++ b/crates/dashboard-core/src/selection.rs @@ -18,9 +18,51 @@ pub fn reselect<'a, K: PartialEq + ?Sized + 'a>( old.min(len.saturating_sub(1)) } +/// What lies past the rows a paged reload has read so far. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Beyond { + /// The listing goes on, and the reload can read its next page. + More, + /// The listing has ended: there is nothing past these rows. + End, + /// The listing goes on, but the reload stopped reading: at its page limit, or on a page that brought nothing or + /// didn't move the cursor on. Whatever is past these rows is unread, not absent. + Unread, +} + +/// What a paged reload does with the rows it has read so far (#91). +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum Reload { + /// Show the rows, with this row selected. + Show(usize), + /// The selected item isn't among them yet and the listing goes on: read the next page first. + More, + /// The selected item isn't among them and the reload stopped before the listing ended. It may only be unread, so + /// the rows can't stand for the list: keep the rows on screen and their selection, and report the reload as failed. + Incomplete, +} + +/// A paged reload keeps the item that was selected before it (`previous`, at row `old`). Found among the rows read so +/// far, the rows show with it selected, whatever lies `beyond`. Not found, it is gone only once the listing has ended +/// (the rows show with [`reselect`]'s row); while the listing goes on the reload reads on, or, stopped, is incomplete. +/// With no `previous` (new filters, or nothing selected), the rows show at once. +pub fn reload<'a, K: PartialEq + ?Sized + 'a>( + previous: Option<&K>, + old: usize, + keys: impl IntoIterator, + beyond: Beyond, +) -> Reload { + let keys: Vec<&K> = keys.into_iter().collect(); + match (previous, beyond) { + (Some(previous), Beyond::More) if !keys.contains(&previous) => Reload::More, + (Some(previous), Beyond::Unread) if !keys.contains(&previous) => Reload::Incomplete, + _ => Reload::Show(reselect(previous, old, keys)), + } +} + #[cfg(test)] mod tests { - use super::reselect; + use super::{Beyond, Reload, reload, reselect}; #[test] fn the_selection_follows_its_row_to_a_new_position() { @@ -50,4 +92,40 @@ mod tests { assert_eq!(reselect(Some("a"), 3, Vec::<&str>::new()), 0); assert_eq!(reselect(None::<&str>, 0, Vec::<&str>::new()), 0); } + + #[test] + fn a_reload_reads_on_while_the_selected_item_is_only_not_read_yet() { + assert_eq!(reload(Some("z"), 55, ["a", "b"], Beyond::More), Reload::More); + } + + #[test] + fn a_reload_shows_the_rows_once_the_selected_item_turns_up() { + for beyond in [Beyond::More, Beyond::End, Beyond::Unread] { + assert_eq!(reload(Some("b"), 55, ["a", "b"], beyond), Reload::Show(1), "{beyond:?}"); + } + } + + #[test] + fn a_listing_that_ended_without_the_item_keeps_the_position() { + assert_eq!(reload(Some("z"), 1, ["a", "b", "c"], Beyond::End), Reload::Show(1)); + assert_eq!(reload(Some("z"), 55, ["a", "b", "c"], Beyond::End), Reload::Show(2)); + assert_eq!(reload(Some("z"), 3, Vec::<&str>::new(), Beyond::End), Reload::Show(0)); + } + + #[test] + fn a_reload_that_stopped_before_the_end_without_the_item_is_incomplete() { + assert_eq!(reload(Some("z"), 55, ["a", "b"], Beyond::Unread), Reload::Incomplete); + assert_eq!( + reload(Some("z"), 0, Vec::<&str>::new(), Beyond::Unread), + Reload::Incomplete + ); + } + + #[test] + fn without_a_selected_item_the_first_rows_show_at_once() { + for beyond in [Beyond::More, Beyond::End, Beyond::Unread] { + assert_eq!(reload(None::<&str>, 0, ["a", "b"], beyond), Reload::Show(0)); + assert_eq!(reload(None::<&str>, 7, ["a", "b"], beyond), Reload::Show(1)); + } + } } diff --git a/crates/dashboard-core/src/work.rs b/crates/dashboard-core/src/work.rs index eb6a8c5..c60b10b 100644 --- a/crates/dashboard-core/src/work.rs +++ b/crates/dashboard-core/src/work.rs @@ -10,6 +10,7 @@ use serde::Deserialize; use serde_json::{Map, Value}; use crate::edit::{Form, Row}; +use crate::selection::Beyond; use crate::write::WriteError; /// The editor's fields, as pmd names them, so a `400` that names one lands next to it. @@ -22,6 +23,8 @@ pub const MAX_REASON: usize = 500; pub const MAX_RANK: i64 = 1_000_000_000; /// Items per page. pmd allows up to 500; a page this size keeps the dialog quick. pub const PAGE: usize = 50; +/// The most pages a reload reads while it looks for the item that was selected (#91): 1,000 items at [`PAGE`]. +pub const FOLLOW_PAGES: usize = 20; /// A status's label, as the Board titles its columns. pub fn status_label(status: &str) -> &str { @@ -137,6 +140,18 @@ impl WorkPage { .map(str::to_owned); Ok(Self { items, next_cursor }) } + + /// What lies past this page for a reload still looking for its selected item, this page read from `cursor` with + /// `pages` pages read so far (#91). Without a next cursor the listing has ended. With one, the reload reads on (from + /// `next_cursor`) only if this page brought at least one item, the cursor moved on and fewer than [`FOLLOW_PAGES`] + /// pages are read, so it can't spin; otherwise the rest of the listing is unread, not absent. + pub fn beyond(&self, cursor: Option<&str>, pages: usize) -> Beyond { + match self.next_cursor.as_deref() { + None => Beyond::End, + Some(next) if self.items.is_empty() || Some(next) == cursor || pages >= FOLLOW_PAGES => Beyond::Unread, + Some(_) => Beyond::More, + } + } } /// `issue` or `pr`, for the kind filter. diff --git a/crates/dashboard-core/tests/work.rs b/crates/dashboard-core/tests/work.rs index ed3f5c1..9391110 100644 --- a/crates/dashboard-core/tests/work.rs +++ b/crates/dashboard-core/tests/work.rs @@ -1,6 +1,7 @@ //! The Work items dialog's data (#35): parsing pmd's items and pages, the list's query, the changed-field `PATCH` and //! its checks, and the write against a loopback stub for every answer pmd gives. use dashboard_core::edit::{Form, Next}; +use dashboard_core::selection::Beyond; use dashboard_core::work::{self, FIELDS, Kind, WorkFilter, WorkItem, WorkPage}; use dashboard_core::write::{Method, Precondition, WriteBackend, WriteError, WriteRequest}; use serde_json::{Value, json}; @@ -71,6 +72,42 @@ fn a_page_reads_its_items_and_an_empty_cursor_means_the_last_page() { ); } +#[test] +fn a_reload_reads_on_only_while_the_listing_moves_on_and_within_its_page_limit() { + let page = |items: usize, next: &str| { + let items: Vec = (0..items).map(|_| item(json!({}))).collect(); + WorkPage::from_json(&json!({"items": items, "nextCursor": next})).unwrap() + }; + assert_eq!(page(50, "50").beyond(None, 1), Beyond::More); + assert_eq!(page(50, "100").beyond(Some("50"), 2), Beyond::More); + assert_eq!(page(12, "").beyond(Some("50"), 2), Beyond::End, "the last page"); + assert_eq!(page(0, "").beyond(Some("50"), 2), Beyond::End, "an empty last page"); + assert_eq!( + page(50, "50").beyond(Some("50"), 2), + Beyond::Unread, + "a cursor that doesn't move on is unread, not the end" + ); + assert_eq!( + page(0, "100").beyond(Some("50"), 2), + Beyond::Unread, + "a page that brings nothing is unread, not the end" + ); + assert_eq!( + page(50, "1000").beyond(Some("950"), work::FOLLOW_PAGES), + Beyond::Unread, + "the page limit is unread, not the end" + ); + assert_eq!( + page(12, "").beyond(Some("950"), work::FOLLOW_PAGES), + Beyond::End, + "a last page at the limit is still the end" + ); + assert_eq!( + page(50, "950").beyond(Some("900"), work::FOLLOW_PAGES - 1), + Beyond::More + ); +} + #[test] fn filters_become_pmds_query_with_paging() { assert_eq!(