From 3567f1b87e6c8bd4facd851de44b45c28e1d920c Mon Sep 17 00:00:00 2001 From: Franz Hemmer Date: Fri, 6 Nov 2026 20:58:30 -0500 Subject: [PATCH 1/2] fix: keep a Work item selected when its save leaves it past the first page (#91) A priority or rank save reloads the Work items list from its first page. When the saved item stayed past that page, the reload didn't contain it, so the selection fell back to row 49 and Back then Enter opened another item. A reload under the same filters now reads on, page by page, until it finds the selected item or the listing ends. Only a listing that ended without it counts as the item leaving the list. The rows on screen and their selection stay until then. The read goes through the existing cursor and generation fences: a filter change or a closed dialog drops a late page, and a failed page keeps the rows on screen beside the error and Retry. It stops on a page that brings nothing or doesn't move the cursor on, and after FOLLOW_PAGES (20) pages. Saves still write once. The decisions are dashboard_core::selection::reload and WorkPage::follow. Co-Authored-By: Claude Opus 5.5 --- crates/dashboard-app/src/captures.rs | 105 +++++++ crates/dashboard-app/src/manage/work_items.rs | 76 +++-- crates/dashboard-app/src/work_items_tests.rs | 275 +++++++++++++++++- crates/dashboard-core/src/selection.rs | 52 +++- crates/dashboard-core/src/work.rs | 12 + crates/dashboard-core/tests/work.rs | 23 ++ 6 files changed, 522 insertions(+), 21 deletions(-) diff --git a/crates/dashboard-app/src/captures.rs b/crates/dashboard-app/src/captures.rs index 27fee91..85e541a 100644 --- a/crates/dashboard-app/src/captures.rs +++ b/crates/dashboard-app/src/captures.rs @@ -1673,3 +1673,108 @@ 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)); +} diff --git a/crates/dashboard-app/src/manage/work_items.rs b/crates/dashboard-app/src/manage/work_items.rs index 7fd279e..1c24225 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,52 @@ 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 follow = page.follow(cursor.as_deref(), pages).map(str::to_owned); + 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()), + follow.is_some(), + ) { + Reload::More => { + work.staged = Some(Staged { rows, pages }); + work.next_cursor = follow; + read_on = true; + } + 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 +343,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..82c1926 100644 --- a/crates/dashboard-app/src/work_items_tests.rs +++ b/crates/dashboard-app/src/work_items_tests.rs @@ -62,6 +62,10 @@ 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, } impl FakeWork { @@ -74,6 +78,8 @@ 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), }) } @@ -111,6 +117,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 @@ -137,7 +147,11 @@ impl WriteBackend for FakeWork { let start: usize = get("cursor").map_or(0, |cursor| cursor.parse().unwrap()); let page: Vec = 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 +1054,262 @@ 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" + ); +} + +#[gpui_kit::test] +async fn a_listing_that_does_not_move_on_ends_the_reload(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; + only_write_is_to(&backend, 255); + assert_eq!( + cursors_since(&backend, before), + [None, Some("50".to_owned())], + "a page that hands back its own cursor ends the reload" + ); + assert_eq!(items(cx, &manage).len(), 100); + click(cx, handle, "work-back"); + assert_eq!(selected(cx, &manage), 55, "a listed row"); + press(cx, handle, "enter"); + assert_eq!(editing(cx, &manage), Some(256)); +} + +/// 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..c419610 100644 --- a/crates/dashboard-core/src/selection.rs +++ b/crates/dashboard-core/src/selection.rs @@ -18,9 +18,35 @@ pub fn reselect<'a, K: PartialEq + ?Sized + 'a>( old.min(len.saturating_sub(1)) } +/// 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, +} + +/// A paged reload keeps the item that was selected before it (`previous`, at row `old`). While that item isn't among +/// the rows read so far and `more` says the listing goes on, it isn't gone, only not read yet: read on. Once it is +/// found, or the listing has ended without it, the rows show with [`reselect`]'s row. With no `previous` (new filters, +/// or nothing selected), the first rows show at once. +pub fn reload<'a, K: PartialEq + ?Sized + 'a>( + previous: Option<&K>, + old: usize, + keys: impl IntoIterator, + more: bool, +) -> Reload { + let keys: Vec<&K> = keys.into_iter().collect(); + match previous { + Some(previous) if more && !keys.contains(&previous) => Reload::More, + _ => Reload::Show(reselect(previous, old, keys)), + } +} + #[cfg(test)] mod tests { - use super::reselect; + use super::{Reload, reload, reselect}; #[test] fn the_selection_follows_its_row_to_a_new_position() { @@ -50,4 +76,28 @@ 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"], true), Reload::More); + } + + #[test] + fn a_reload_shows_the_rows_once_the_selected_item_turns_up() { + assert_eq!(reload(Some("b"), 55, ["a", "b"], true), Reload::Show(1)); + assert_eq!(reload(Some("b"), 55, ["a", "b"], false), Reload::Show(1)); + } + + #[test] + fn a_listing_that_ended_without_the_item_keeps_the_position() { + assert_eq!(reload(Some("z"), 1, ["a", "b", "c"], false), Reload::Show(1)); + assert_eq!(reload(Some("z"), 55, ["a", "b", "c"], false), Reload::Show(2)); + assert_eq!(reload(Some("z"), 3, Vec::<&str>::new(), false), Reload::Show(0)); + } + + #[test] + fn without_a_selected_item_the_first_rows_show_at_once() { + assert_eq!(reload(None::<&str>, 0, ["a", "b"], true), Reload::Show(0)); + assert_eq!(reload(None::<&str>, 7, ["a", "b"], true), Reload::Show(1)); + } } diff --git a/crates/dashboard-core/src/work.rs b/crates/dashboard-core/src/work.rs index eb6a8c5..e402502 100644 --- a/crates/dashboard-core/src/work.rs +++ b/crates/dashboard-core/src/work.rs @@ -22,6 +22,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 +139,16 @@ impl WorkPage { .map(str::to_owned); Ok(Self { items, next_cursor }) } + + /// Where a reload still looking for its selected item reads on from, after this page, read from `cursor`, made + /// `pages` pages so far (#91). `None` ends the reload: the listing is done, the page brought nothing or didn't move + /// the cursor on, or [`FOLLOW_PAGES`] pages have been read. Each page read on brings at least one item and a new + /// cursor, so the reload can't spin. + pub fn follow(&self, cursor: Option<&str>, pages: usize) -> Option<&str> { + self.next_cursor + .as_deref() + .filter(|next| !self.items.is_empty() && Some(*next) != cursor && pages < FOLLOW_PAGES) + } } /// `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..fed322d 100644 --- a/crates/dashboard-core/tests/work.rs +++ b/crates/dashboard-core/tests/work.rs @@ -71,6 +71,29 @@ 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").follow(None, 1), Some("50")); + assert_eq!(page(50, "100").follow(Some("50"), 2), Some("100")); + assert_eq!(page(12, "").follow(Some("50"), 2), None, "the last page"); + assert_eq!( + page(50, "50").follow(Some("50"), 2), + None, + "a cursor that doesn't move on" + ); + assert_eq!(page(0, "100").follow(Some("50"), 2), None, "a page that brings nothing"); + assert_eq!( + page(50, "1000").follow(Some("950"), work::FOLLOW_PAGES), + None, + "the page limit" + ); + assert_eq!(page(50, "950").follow(Some("900"), work::FOLLOW_PAGES - 1), Some("950")); +} + #[test] fn filters_become_pmds_query_with_paging() { assert_eq!( From 456ae3ef02b5277ea8b9976af092ed69d3e6768d Mon Sep 17 00:00:00 2001 From: Franz Hemmer Date: Fri, 6 Nov 2026 21:46:39 -0500 Subject: [PATCH 2/2] fix: report a reload that stops before the list ends instead of reselecting (#91) The reload that looks for the saved Work item stopped on four signals: the last page, the page limit, an empty page and a cursor that doesn't move on. It treated all four as the end of the list, so at the limit or on a stalled listing a still-unread item counted as gone and the selection moved to another one. WorkPage::beyond now tells a real end (no next cursor) from a stop that leaves the rest unread, and selection::reload answers Incomplete when the item isn't found before such a stop. The dialog then keeps the rows on screen and their selection, as for a failed page, and shows the error beside Retry. Found on the last page it reads, the item still shows; a real end without it still falls back to the row in its place. Co-Authored-By: Claude Opus 5.5 --- crates/dashboard-app/src/captures.rs | 108 +++++++++++++++ crates/dashboard-app/src/manage/work_items.rs | 18 ++- crates/dashboard-app/src/work_items_tests.rs | 125 ++++++++++++++++-- crates/dashboard-core/src/selection.rs | 60 ++++++--- crates/dashboard-core/src/work.rs | 19 +-- crates/dashboard-core/tests/work.rs | 36 +++-- 6 files changed, 320 insertions(+), 46 deletions(-) diff --git a/crates/dashboard-app/src/captures.rs b/crates/dashboard-app/src/captures.rs index 85e541a..7557d2f 100644 --- a/crates/dashboard-app/src/captures.rs +++ b/crates/dashboard-app/src/captures.rs @@ -1778,3 +1778,111 @@ fn captures_work_deep_page_selection() { 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 1c24225..7cb0f5c 100644 --- a/crates/dashboard-app/src/manage/work_items.rs +++ b/crates/dashboard-app/src/manage/work_items.rs @@ -296,7 +296,7 @@ impl Manage { Some(staged) => (staged.rows, staged.pages + 1), None => (Vec::new(), 1), }; - let follow = page.follow(cursor.as_deref(), pages).map(str::to_owned); + 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 @@ -309,13 +309,25 @@ impl Manage { previous.as_deref(), work.selected, rows.iter().map(|item| item.id.as_str()), - follow.is_some(), + beyond, ) { Reload::More => { work.staged = Some(Staged { rows, pages }); - work.next_cursor = follow; + 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; diff --git a/crates/dashboard-app/src/work_items_tests.rs b/crates/dashboard-app/src/work_items_tests.rs index 82c1926..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}; @@ -66,6 +66,8 @@ pub(crate) struct FakeWork { 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 { @@ -80,6 +82,7 @@ impl FakeWork { in_pmd_order: AtomicBool::new(false), fail_next_page: Mutex::default(), stuck_cursor: AtomicBool::new(false), + empty_pages: AtomicBool::new(false), }) } @@ -145,7 +148,11 @@ 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() { if paged && self.stuck_cursor.load(Ordering::SeqCst) { start.to_string() @@ -1213,8 +1220,57 @@ async fn a_later_page_that_fails_keeps_the_rows_and_retry_finds_the_item(cx: &mu ); } +/// 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_ends_the_reload(cx: &mut TestAppContext) { +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; @@ -1224,17 +1280,70 @@ async fn a_listing_that_does_not_move_on_ends_the_reload(cx: &mut TestAppContext let before = lists(&backend); click(cx, handle, "work-save"); settle(cx, handle, &view).await; - only_write_is_to(&backend, 255); + 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 ends the reload" + "a page that hands back its own cursor stops the reload" ); - assert_eq!(items(cx, &manage).len(), 100); +} + +#[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"); - assert_eq!(selected(cx, &manage), 55, "a listed row"); press(cx, handle, "enter"); - assert_eq!(editing(cx, &manage), Some(256)); + 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. diff --git a/crates/dashboard-core/src/selection.rs b/crates/dashboard-core/src/selection.rs index c419610..0a24c35 100644 --- a/crates/dashboard-core/src/selection.rs +++ b/crates/dashboard-core/src/selection.rs @@ -18,6 +18,18 @@ 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 { @@ -25,28 +37,32 @@ pub enum Reload { 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`). While that item isn't among -/// the rows read so far and `more` says the listing goes on, it isn't gone, only not read yet: read on. Once it is -/// found, or the listing has ended without it, the rows show with [`reselect`]'s row. With no `previous` (new filters, -/// or nothing selected), the first rows show at once. +/// 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, - more: bool, + beyond: Beyond, ) -> Reload { let keys: Vec<&K> = keys.into_iter().collect(); - match previous { - Some(previous) if more && !keys.contains(&previous) => Reload::More, + 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::{Reload, reload, reselect}; + use super::{Beyond, Reload, reload, reselect}; #[test] fn the_selection_follows_its_row_to_a_new_position() { @@ -79,25 +95,37 @@ mod tests { #[test] fn a_reload_reads_on_while_the_selected_item_is_only_not_read_yet() { - assert_eq!(reload(Some("z"), 55, ["a", "b"], true), Reload::More); + 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() { - assert_eq!(reload(Some("b"), 55, ["a", "b"], true), Reload::Show(1)); - assert_eq!(reload(Some("b"), 55, ["a", "b"], false), Reload::Show(1)); + 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"], false), Reload::Show(1)); - assert_eq!(reload(Some("z"), 55, ["a", "b", "c"], false), Reload::Show(2)); - assert_eq!(reload(Some("z"), 3, Vec::<&str>::new(), false), Reload::Show(0)); + 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() { - assert_eq!(reload(None::<&str>, 0, ["a", "b"], true), Reload::Show(0)); - assert_eq!(reload(None::<&str>, 7, ["a", "b"], true), Reload::Show(1)); + 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 e402502..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. @@ -140,14 +141,16 @@ impl WorkPage { Ok(Self { items, next_cursor }) } - /// Where a reload still looking for its selected item reads on from, after this page, read from `cursor`, made - /// `pages` pages so far (#91). `None` ends the reload: the listing is done, the page brought nothing or didn't move - /// the cursor on, or [`FOLLOW_PAGES`] pages have been read. Each page read on brings at least one item and a new - /// cursor, so the reload can't spin. - pub fn follow(&self, cursor: Option<&str>, pages: usize) -> Option<&str> { - self.next_cursor - .as_deref() - .filter(|next| !self.items.is_empty() && Some(*next) != cursor && pages < FOLLOW_PAGES) + /// 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, + } } } diff --git a/crates/dashboard-core/tests/work.rs b/crates/dashboard-core/tests/work.rs index fed322d..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}; @@ -77,21 +78,34 @@ fn a_reload_reads_on_only_while_the_listing_moves_on_and_within_its_page_limit() let items: Vec = (0..items).map(|_| item(json!({}))).collect(); WorkPage::from_json(&json!({"items": items, "nextCursor": next})).unwrap() }; - assert_eq!(page(50, "50").follow(None, 1), Some("50")); - assert_eq!(page(50, "100").follow(Some("50"), 2), Some("100")); - assert_eq!(page(12, "").follow(Some("50"), 2), None, "the last page"); + 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").follow(Some("50"), 2), - None, - "a cursor that doesn't move on" + 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").follow(Some("50"), 2), None, "a page that brings nothing"); assert_eq!( - page(50, "1000").follow(Some("950"), work::FOLLOW_PAGES), - None, - "the page limit" + 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 ); - assert_eq!(page(50, "950").follow(Some("900"), work::FOLLOW_PAGES - 1), Some("950")); } #[test]